|
|
Patch Set 1 #
Total comments: 14
Patch Set 2 : Factored CMake flags #Patch Set 3 : Updated CMake Script & Fixed AIX RTTI Detection #Patch Set 4 : Added incorrectly removed comment #
Total messages: 2
|
wan
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 ...
|
15 years, 9 months ago (2010年03月23日 17:29:30 UTC) #1 | |||||||||||||||||||||||||||||||
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}.
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}.