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

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