Google Test AIX/Solaris Porting Effort

[wan, 2010-03-23 17:29:30]

http://codereview.appspot.com/656044/diff/1/4
File CMakeLists.txt (right):

http://codereview.appspot.com/656044/diff/1/4#newcode77
CMakeLists.txt:77: set(cxx_base "${CMAKE_CXX_FLAGS} ${cxx_base}")
Remove ${cxx_base} from the definition body - it's not defined yet.

http://codereview.appspot.com/656044/diff/1/4#newcode79
CMakeLists.txt:79: set(cxx_default "${cxx_base} ${cxx_exceptions}")
Where is cxx_exceptions defined?

http://codereview.appspot.com/656044/diff/1/4#newcode80
CMakeLists.txt:80: set(cxx_strict "${cxx_default} ${cxx_strict}")
Remove ${cxx_strict}.

http://codereview.appspot.com/656044/diff/1/4#newcode84
CMakeLists.txt:84: if (UNIX AND CMAKE_USE_PTHREADS_INIT)  # The pthreads library
is available.
Why is 'UNIX' necessary here?

http://codereview.appspot.com/656044/diff/1/4#newcode85
CMakeLists.txt:85: set(cxx_base "${cxx_base} -DGTEST_HAS_PTHREAD=1")
cxx_base must be defined before cxx_default and cxx_strict, as the latter two
depend on the value of cxx_base.

http://codereview.appspot.com/656044/diff/1/4#newcode259
CMakeLists.txt:259: set(cxx_no_exception "${cxx_base} ${cxx_no_exception}")
Remove ${cxx_no_exception}.

http://codereview.appspot.com/656044/diff/1/4#newcode260
CMakeLists.txt:260: set(cxx_no_rtti "${cxx_default} ${cxx_no_rtti}
-DGTEST_HAS_RTTI=0")
Remove ${cxx_no_rtti}.

[Hady, 2010-03-23 18:26:08]

I've published my comments. I'm not quite sure why you want me to remove the
cxx_no_exception and cxx_no_rtti flags. I put them there to be able and set my
toolchain's equivalent of '-fno-exceptions' and '-fno-rtti'. Maybe the naming is
off? I uploaded a patch set which I think is better. I basically factored out
all the common parts leaving it to the user to define certain flags on
non-supported platforms. Let me know what you think.

http://codereview.appspot.com/656044/diff/1/4
File CMakeLists.txt (right):

http://codereview.appspot.com/656044/diff/1/4#newcode77
CMakeLists.txt:77: set(cxx_base "${CMAKE_CXX_FLAGS} ${cxx_base}")
Ok will do.
On 2010/03/23 17:29:30, wan wrote:
> Remove ${cxx_base} from the definition body - it's not defined yet.

http://codereview.appspot.com/656044/diff/1/4#newcode79
CMakeLists.txt:79: set(cxx_default "${cxx_base} ${cxx_exceptions}")
It's not. It'll be empty "" or set by the user on the command line like so:
-Dcxx_exceptions="-qeh"
On 2010/03/23 17:29:30, wan wrote:
> Where is cxx_exceptions defined?

http://codereview.appspot.com/656044/diff/1/4#newcode80
CMakeLists.txt:80: set(cxx_strict "${cxx_default} ${cxx_strict}")
Are you sure? What if a toolchain has the equivalent?
On 2010/03/23 17:29:30, wan wrote:
> Remove ${cxx_strict}.

http://codereview.appspot.com/656044/diff/1/4#newcode84
CMakeLists.txt:84: if (UNIX AND CMAKE_USE_PTHREADS_INIT)  # The pthreads library
is available.
It's not, was just too worried about breaking your Win32 build. Would you like
me to remove it?
On 2010/03/23 17:29:30, wan wrote:
> Why is 'UNIX' necessary here?

http://codereview.appspot.com/656044/diff/1/4#newcode85
CMakeLists.txt:85: set(cxx_base "${cxx_base} -DGTEST_HAS_PTHREAD=1")
Absolutely. Will pull up the thread test and try to merge the gnu and non-gnu
branches...
On 2010/03/23 17:29:30, wan wrote:
> cxx_base must be defined before cxx_default and cxx_strict, as the latter two
> depend on the value of cxx_base.

http://codereview.appspot.com/656044/diff/1/4#newcode259
CMakeLists.txt:259: set(cxx_no_exception "${cxx_base} ${cxx_no_exception}")
Same as above. What if the user wants to test the features with his 'no
exceptions' flag?
On 2010/03/23 17:29:30, wan wrote:
> Remove ${cxx_no_exception}.

http://codereview.appspot.com/656044/diff/1/4#newcode260
CMakeLists.txt:260: set(cxx_no_rtti "${cxx_default} ${cxx_no_rtti}
-DGTEST_HAS_RTTI=0")
Same here...
On 2010/03/23 17:29:30, wan wrote:
> Remove ${cxx_no_rtti}.