List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:December 14 2010 10:10am
Subject:Re: bzr commit into mysql-5.5-bugteam branch (davi:3188) Bug#58871
View as plain text  
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".

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.

>
>> +    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
Thread
bzr commit into mysql-5.5-bugteam branch (davi:3188) Bug#58871Davi Arnaut13 Dec
Re: bzr commit into mysql-5.5-bugteam branch (davi:3188) Bug#58871Tor Didriksen14 Dec
  • Re: bzr commit into mysql-5.5-bugteam branch (davi:3188) Bug#58871Davi Arnaut14 Dec
    • Re: bzr commit into mysql-5.5-bugteam branch (davi:3188) Bug#58871Tor Didriksen14 Dec
      • Re: bzr commit into mysql-5.5-bugteam branch (davi:3188) Bug#58871Davi Arnaut14 Dec