Code review - Issue 8690: Fix build on Linux IA-64https://codereview.appspot.com/2008-12-02T20:09:44+00:00rietveld
Message from unknown
2008-12-01T15:04:41+00:00Hobbesurn:md5:48190e599005b42950950fb1b00dcd89
Message from schrei99@gmail.com
2008-12-01T15:04:41+00:00Hobbesurn:md5:53d656e5b5c2e0214334c23e4bb2e5e4
Message from chandlerc@gmail.com
2008-12-02T18:09:59+00:00chandlercurn:md5:0a466b1ecdf13da16a3456b9284414e2
Tiny nit, but otherwise this looks good.
http://codereview.appspot.com/8690/diff/1/2
File include/gtest/internal/gtest-port.h (right):
http://codereview.appspot.com/8690/diff/1/2#newcode332
Line 332: #if GTEST_HAS_STD_STRING && defined(GTEST_OS_LINUX) && GTEST_HAS_CLONE
Can we group the GTEST_HAS_* checks? I'd put the OS check first, but either way. Just feels odd with them split.
Message from unknown
2008-12-02T18:16:05+00:00Hobbesurn:md5:bf261f92a2e065f8fe1384e672d6a615
Message from schrei99@gmail.com
2008-12-02T18:16:05+00:00Hobbesurn:md5:8a55cd38adfc6487aff825099bd75e94
reordered checks
Message from unknown
2008-12-02T18:18:12+00:00Hobbesurn:md5:bce00cd6f4afca52fe5e51f40984508a
Message from schrei99@gmail.com
2008-12-02T18:18:12+00:00Hobbesurn:md5:a6b7fc14b919905d1c6d21bf011e28b6
and now fixing the newlines
Message from shiqian@gmail.com
2008-12-02T18:27:07+00:00Zhanyongurn:md5:68b82ff219d17220ac4afac9eca8885e
http://codereview.appspot.com/8690/diff/9/10
File include/gtest/internal/gtest-port.h (right):
http://codereview.appspot.com/8690/diff/9/10#newcode319
Line 319: // On IA-64 for example it isn't: see http://linux.die.net/man/2/clone2
Please also say that non-Linux systems usually don't support it.
Nit: comments should be English sentences that end with a period.
http://codereview.appspot.com/8690/diff/9/10#newcode326
Line 326: #define GTEST_HAS_CLONE 1
clone() is a Linux thing. We cannot define this to 1 if we are not on Linux. Please define it to 1 only if we are on Linux and __ia64__ is not defined.
Add an entry for this macro in the big block comment above (line 43). Next to other GTEST_HAS_* macros. Now that the list is longer, we should sort it alphabetically.
http://codereview.appspot.com/8690/diff/9/10#newcode332
Line 332: #if defined(GTEST_OS_LINUX) && GTEST_HAS_STD_STRING && GTEST_HAS_CLONE
Remove GTEST_OS_LINUX after you fix the definition of GTEST_HAS_CLONE.
Message from unknown
2008-12-02T19:09:06+00:00Hobbesurn:md5:ad84623b66ef364c52b48a72b8254161
Message from schrei99@gmail.com
2008-12-02T19:09:07+00:00Hobbesurn:md5:7e5786b6ffd697486a23cf4670607f25
fixing comments, reordering GTEST_HAS_ macros, only defining GTEST_HAS_CLONE on Linux/non-IA64
Message from chandlerc@gmail.com
2008-12-02T19:16:29+00:00chandlercurn:md5:6ea9d59fbb57d603ba8263e475239555
LGTM
Much nicer, thanks!
http://codereview.appspot.com/8690/diff/14/15
File include/gtest/internal/gtest-port.h (right):
http://codereview.appspot.com/8690/diff/14/15#newcode43
Line 43: // GTEST_HAS_CLONE - Define it to 1/0 to indicate clone(2)
indicate _that_ clone(2) ...
Message from shiqian@gmail.com
2008-12-02T20:01:45+00:00Zhanyongurn:md5:fb66ca26ea6e315c534c57696911a7fc
Looks good.
I'll do some testing and then commit it to trunk. Cheers!
http://codereview.appspot.com/8690/diff/14/15
File include/gtest/internal/gtest-port.h (right):
http://codereview.appspot.com/8690/diff/14/15#newcode43
Line 43: // GTEST_HAS_CLONE - Define it to 1/0 to indicate clone(2)
On 2008/12/02 19:16:29, chandlerc wrote:
> indicate _that_ clone(2) ...
I can fix this when I commit the patch.
Message from schrei99@gmail.com
2008-12-02T20:09:44+00:00Hobbesurn:md5:2d3b08257320774676802c40774be6ef
http://codereview.appspot.com/8690/diff/14/15
File include/gtest/internal/gtest-port.h (right):
http://codereview.appspot.com/8690/diff/14/15#newcode43
Line 43: // GTEST_HAS_CLONE - Define it to 1/0 to indicate clone(2)
On 2008/12/02 20:01:46, Zhanyong wrote:
> On 2008/12/02 19:16:29, chandlerc wrote:
> > indicate _that_ clone(2) ...
>
> I can fix this when I commit the patch.
Thanks. You might wanna fix GTEST_HAS_TR1_TUPLE as well.