On Tue, Dec 14, 2010 at 11:10 AM, Davi Arnaut <davi.arnaut@stripped>wrote:
> Hi Tor,
>
>
> On 12/14/10 7:13 AM, Tor Didriksen wrote:
>
>> Hi Davi
>>
>> Inline comments below.
>>
>> -- didrik
>>
>> On Mon, Dec 13, 2010 at 6:36 PM, Davi Arnaut<davi.arnaut@stripped>
>> wrote:
>>
>> # At a local mysql-5.5-bugteam repository of davi
>>>
>>> 3188 Davi Arnaut 2010-12-13
>>> Bug#58871: Reorganize maintainer mode compiler flags to allow
>>> option for specific compilers
>>>
>>> Reorganize the maintainer mode cmake code to allow options
>>> for specific compilers. For now, enable -Wall for ICC, but
>>> do not turn warnings into errors.
>>> @ CMakeLists.txt
>>> Move the code that sets options to cmake/maintainer.cmake
>>> @ cmake/maintainer.cmake
>>> Add macros for each specific compiler.
>>>
>>> added:
>>> cmake/maintainer.cmake
>>> modified:
>>> CMakeLists.txt
>>> === modified file 'CMakeLists.txt'
>>> --- a/CMakeLists.txt 2010-11-24 10:23:44 +0000
>>> +++ b/CMakeLists.txt 2010-12-13 17:36:41 +0000
>>> @@ -115,30 +115,22 @@ ENDIF()
>>> # Control aspects of the development environment which are
>>> # specific to MySQL maintainers and developers.
>>> #
>>> -INCLUDE (CheckCCompilerFlag)
>>> +INCLUDE(maintainer)
>>> OPTION(MYSQL_MAINTAINER_MODE "MySQL maintainer-specific development
>>> environment" OFF)
>>>
>>>
>> please fix this long line
>>
>
> Fixed.
>
>
>
>> -# Whether the maintainer mode should be enabled.
>>> +
>>> +# Whether the maintainer mode compiler options should be enabled.
>>> IF(MYSQL_MAINTAINER_MODE)
>>> - IF(CMAKE_COMPILER_IS_GNUCC)
>>> - CHECK_C_COMPILER_FLAG("-Wdeclaration-after-statement"
>>> HAVE_DECLARATION_AFTER_STATEMENT)
>>> - IF(HAVE_DECLARATION_AFTER_STATEMENT)
>>> - SET(MY_MAINTAINER_DECLARATION_AFTER_STATEMENT
>>> "-Wdeclaration-after-statement")
>>> - ENDIF()
>>> - SET(MY_MAINTAINER_C_WARNINGS
>>> - "-Wall -Wextra -Wunused -Wwrite-strings -Wno-strict-aliasing
>>> -Werror")
>>> + IF("${CMAKE_C_COMPILER_ID}" MATCHES "GNU")
>>>
>>>
>> remove the variable substitution and quoting, here and below:
>>
>> IF(CMAKE_C_COMPILER_ID MATCHES "GNU")
>>
>> CMAKE_C_COMPILER_ID seems to be an internal cmake variable,
>> but we are already using it elsewhere, so I guess it is ok.
>>
>> There's no regex on the right-hand-side, so you might as well write
>>
>> IF(CMAKE_C_COMPILER_ID STREQUAL "GNU")
>>
>>
> Hum, I gather this might be a matter of taste. I prefer the ${VAR} style as
> its the preferred method by CMake and recommended in their manual. Anyway,
> VAR in a IF expression is treated as ${VAR}. The CMake manual even says that
> ${VAR} "is really the correct way to write it".
>
let us try to keep a consistent style.
looking at the rest of the code, we always write IF(VAR) rather than
IF(${VAR}) or IF ("${VAR}")
>
> As for using STREQUAL, I think we should stick to the MATCHES variant
> because its the way used throughout the CMake code (which is where I based
> those checks on). There is not much to gain by just being different.
fine with me
-- didrik
>
>
>
>> + SET_MYSQL_MAINTAINER_GNU_C_OPTIONS()
>>> ENDIF()
>>> - IF(CMAKE_COMPILER_IS_GNUCXX)
>>> - SET(MY_MAINTAINER_CXX_WARNINGS "${MY_MAINTAINER_C_WARNINGS}
>>> -Wno-unused-parameter"
>>> - CACHE STRING "C++ warning options used in maintainer builds.")
>>> + IF("${CMAKE_CXX_COMPILER_ID}" MATCHES "GNU")
>>> + SET_MYSQL_MAINTAINER_GNU_CXX_OPTIONS()
>>> ENDIF()
>>> - IF(CMAKE_COMPILER_IS_GNUCC)
>>> - SET(MY_MAINTAINER_C_WARNINGS
>>> - "${MY_MAINTAINER_C_WARNINGS}
>>> ${MY_MAINTAINER_DECLARATION_AFTER_STATEMENT}"
>>> - CACHE STRING "C warning options used in maintainer builds.")
>>> + IF("${CMAKE_C_COMPILER_ID}" MATCHES "Intel")
>>> + SET_MYSQL_MAINTAINER_INTEL_C_OPTIONS()
>>> ENDIF()
>>> - # Do not make warnings in checks into errors.
>>> - IF(CMAKE_COMPILER_IS_GNUCC AND CMAKE_COMPILER_IS_GNUCXX)
>>> - SET(CMAKE_REQUIRED_FLAGS "${CMAKE_REQUIRED_FLAGS} -Wno-error")
>>> + IF("${CMAKE_CXX_COMPILER_ID}" MATCHES "Intel")
>>> + SET_MYSQL_MAINTAINER_INTEL_CXX_OPTIONS()
>>> ENDIF()
>>> ENDIF()
>>>
>>>
>>>
>> Please fix the long lines in the file below.
>>
>>
> Fixed.
>
> Davi
>
