mroonga / mroonga

A MySQL pluggable storage engine based on Groonga
http://mroonga.org/
GNU Lesser General Public License v2.1
182 stars 37 forks source link

Checks for C/C++ compiler flags are skipped #21

Closed s-yata closed 10 years ago

s-yata commented 10 years ago

Description

Checks for C/C++ compiler flags are skipped in CMake.

This is because the same variable "is_available" is used for all checks. check_c/cxx_compiler_flag() skips a check when "is_available" is already set. This means that only the first check is valid.

The original comment is as follows:

In mroonga CMakeLists.txt there are similar macros. They also print
too much, but they're completely broken - they use the same variable
(is_available) for all flags, and because of caching only the first
check actually works. No doubt you know that and that's why in groonga
you generate unique temporary_variable_name for every flag.
Perhaps you've forgot to fix mroonga, but luckily, these macros are
never used in bundled mroonga.

Solution

MariaDB developers also gave us a comment as follows:

I'd suggest to remove your message(STATUS) and also, for simplicity, you
may want to remove the code that creates temporary_variable_name. You
can simply use "HAVE_${flag}" like TokuDB does (and we do in 10.1). The
result will look like

I found an example in mariadb-10.1.0/cmake/check_compiler_flag.cmake. MariaDB-10.1.0 uses HAVEC/CXX*.

MY_CHECK_C_COMPILER_FLAG(${flag} HAVE_C_${flag})
MY_CHECK_CXX_COMPILER_FLAG(${flag} HAVE_CXX_${flag})

We can use the same variable names to skip checks for C/C++ compiler flags.

s-yata commented 10 years ago

I've run CMake and gotten the following output. It seems that only the first check (-Wall for C compiler) is valid.

...
-- Performing Test is_available
-- Performing Test is_available - Success
-- checking for C flag '-Wall' - available
-- checking for CXX flag '-Wall' - available
-- checking for C flag '-Wextra' - available
-- checking for CXX flag '-Wextra' - available
-- checking for C flag '-Wno-unused-parameter' - available
-- checking for CXX flag '-Wno-unused-parameter' - available
-- checking for C flag '-Wno-strict-aliasing' - available
-- checking for CXX flag '-Wno-strict-aliasing' - available
-- checking for C flag '-Wno-deprecated' - available
-- checking for CXX flag '-Wno-deprecated' - available
-- checking for CXX flag '-fno-implicit-templates' - available
-- checking for CXX flag '-fno-exceptions' - available
-- checking for CXX flag '-fno-rtti' - available
-- checking for CXX flag '-felide-constructors' - available
...
s-yata commented 10 years ago

As explained in #22, we feel that "Performing Test HAVE_-Wno-clobbered" is strange. We prefer "Performing Test CXXFLAG_WNO_CLOBBERED" (Groonga style).

s-yata commented 10 years ago

Output of CMake changed as follows:

...
-- Performing Test CFLAG_WALL
-- Performing Test CFLAG_WALL - Success
-- Performing Test CXXFLAG_WALL
-- Performing Test CXXFLAG_WALL - Success
-- Performing Test CFLAG_WEXTRA
-- Performing Test CFLAG_WEXTRA - Success
-- Performing Test CXXFLAG_WEXTRA
-- Performing Test CXXFLAG_WEXTRA - Success
-- Performing Test CFLAG_WNO_UNUSED_PARAMETER
-- Performing Test CFLAG_WNO_UNUSED_PARAMETER - Success
-- Performing Test CXXFLAG_WNO_UNUSED_PARAMETER
-- Performing Test CXXFLAG_WNO_UNUSED_PARAMETER - Success
-- Performing Test CFLAG_WNO_STRICT_ALIASING
-- Performing Test CFLAG_WNO_STRICT_ALIASING - Success
-- Performing Test CXXFLAG_WNO_STRICT_ALIASING
-- Performing Test CXXFLAG_WNO_STRICT_ALIASING - Success
-- Performing Test CFLAG_WNO_DEPRECATED
-- Performing Test CFLAG_WNO_DEPRECATED - Success
-- Performing Test CXXFLAG_WNO_DEPRECATED
-- Performing Test CXXFLAG_WNO_DEPRECATED - Success
-- Performing Test CXXFLAG_FNO_IMPLICIT_TEMPLATES
-- Performing Test CXXFLAG_FNO_IMPLICIT_TEMPLATES - Success
-- Performing Test CXXFLAG_FNO_EXCEPTIONS
-- Performing Test CXXFLAG_FNO_EXCEPTIONS - Success
-- Performing Test CXXFLAG_FNO_RTTI
-- Performing Test CXXFLAG_FNO_RTTI - Success
-- Performing Test CXXFLAG_FELIDE_CONSTRUCTORS
-- Performing Test CXXFLAG_FELIDE_CONSTRUCTORS - Success
...