You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by we...@apache.org on 2019/04/02 16:24:49 UTC
[arrow] branch master updated: ARROW-5088: [C++] Only add -Werror
in debug builds. Add C++ documentation about compiler warning levels
This is an automated email from the ASF dual-hosted git repository.
wesm pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/master by this push:
new 9af4132 ARROW-5088: [C++] Only add -Werror in debug builds. Add C++ documentation about compiler warning levels
9af4132 is described below
commit 9af4132772072d5a891ddaf4135c2ee91f51946c
Author: Wes McKinney <we...@apache.org>
AuthorDate: Tue Apr 2 11:24:38 2019 -0500
ARROW-5088: [C++] Only add -Werror in debug builds. Add C++ documentation about compiler warning levels
I think there are two reasonable options:
* Only add `-Werror` in debug builds
* Disallow use of BUILD_WARNING_LEVEL=CHECKIN in release builds altogether
I think it's useful at least to be able to _see_ the warnings, so I went with the first option.
Author: Wes McKinney <we...@apache.org>
Closes #4093 from wesm/ARROW-5088 and squashes the following commits:
7689a1483 <Wes McKinney> Grammar
15fc98efc <Wes McKinney> Only add -Werror in debug builds. Add C++ documentation about compiler warning levels
---
cpp/cmake_modules/SetupCxxFlags.cmake | 35 +++++++++++++++++++----------------
docs/source/developers/cpp.rst | 12 ++++++++++++
2 files changed, 31 insertions(+), 16 deletions(-)
diff --git a/cpp/cmake_modules/SetupCxxFlags.cmake b/cpp/cmake_modules/SetupCxxFlags.cmake
index e9d2695..3ce2dc8 100644
--- a/cpp/cmake_modules/SetupCxxFlags.cmake
+++ b/cpp/cmake_modules/SetupCxxFlags.cmake
@@ -101,8 +101,11 @@ else()
endif()
# BUILD_WARNING_LEVEL add warning/error compiler flags. The possible values are
-# - RELEASE: `-Werror` is not provide, thus warning do not halt the build.
-# - CHECKIN: Imply `-Werror -Wall` and some other warnings.
+# - PRODUCTION: Build with `-Wall` but do not add `-Werror`, so warnings do not
+# halt the build.
+# - CHECKIN: Imply `-Weverything` with certain pedantic warnings
+# disabled. `-Werror` is added in debug builds so that any warnings fail the
+# build
# - EVERYTHING: Like `CHECKIN`, but possible extra flags depending on the
# compiler, including `-Wextra`, `-Weverything`, `-pedantic`.
# This is the most aggressive warning level.
@@ -121,13 +124,22 @@ string(TOUPPER ${BUILD_WARNING_LEVEL} BUILD_WARNING_LEVEL)
message(STATUS "Arrow build warning level: ${BUILD_WARNING_LEVEL}")
+macro(arrow_add_werror_if_debug)
+ if("${CMAKE_BUILD_TYPE}" STREQUAL "DEBUG")
+ # Treat all compiler warnings as errors
+ if("${COMPILER_FAMILY}" STREQUAL "msvc")
+ set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /WX")
+ else()
+ set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Werror")
+ endif()
+ endif()
+endmacro()
+
if("${BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN")
# Pre-checkin builds
if("${COMPILER_FAMILY}" STREQUAL "msvc")
string(REPLACE "/W3" "" CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS}")
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /W3")
- # Treat all compiler warnings as errors
- set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /WX")
elseif("${COMPILER_FAMILY}" STREQUAL "clang")
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Weverything -Wno-c++98-compat \
-Wno-c++98-compat-pedantic -Wno-deprecated -Wno-weak-vtables -Wno-padded \
@@ -159,18 +171,14 @@ if("${BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN")
if("${COMPILER_VERSION}" VERSION_GREATER "3.9")
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-zero-as-null-pointer-constant")
endif()
-
- # Treat all compiler warnings as errors
- set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-unknown-warning-option -Werror")
+ set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wno-unknown-warning-option")
elseif("${COMPILER_FAMILY}" STREQUAL "gcc")
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Wall \
-Wno-conversion -Wno-sign-conversion -Wno-unused-variable")
-
- # Treat all compiler warnings as errors
- set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Werror")
else()
message(FATAL_ERROR "Unknown compiler. Version info:\n${COMPILER_VERSION_FULL}")
endif()
+ arrow_add_werror_if_debug()
elseif("${BUILD_WARNING_LEVEL}" STREQUAL "EVERYTHING")
# Pedantic builds for fixing warnings
if("${COMPILER_FAMILY}" STREQUAL "msvc")
@@ -178,21 +186,16 @@ elseif("${BUILD_WARNING_LEVEL}" STREQUAL "EVERYTHING")
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /Wall")
# https://docs.microsoft.com/en-us/cpp/build/reference/compiler-option-warning-level
# /wdnnnn disables a warning where "nnnn" is a warning number
- # Treat all compiler warnings as errors
- set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /WX")
elseif("${COMPILER_FAMILY}" STREQUAL "clang")
set(CXX_COMMON_FLAGS
"${CXX_COMMON_FLAGS} -Weverything -Wno-c++98-compat -Wno-c++98-compat-pedantic")
- # Treat all compiler warnings as errors
- set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Werror")
elseif("${COMPILER_FAMILY}" STREQUAL "gcc")
set(CXX_COMMON_FLAGS
"${CXX_COMMON_FLAGS} -Wall -Wpedantic -Wextra -Wno-unused-parameter")
- # Treat all compiler warnings as errors
- set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Werror")
else()
message(FATAL_ERROR "Unknown compiler. Version info:\n${COMPILER_VERSION_FULL}")
endif()
+ arrow_add_werror_if_debug()
else()
# Production builds (warning are not treated as errors)
if("${COMPILER_FAMILY}" STREQUAL "msvc")
diff --git a/docs/source/developers/cpp.rst b/docs/source/developers/cpp.rst
index 9025bce..71ee003 100644
--- a/docs/source/developers/cpp.rst
+++ b/docs/source/developers/cpp.rst
@@ -302,6 +302,18 @@ C++ codebase.
features or developer tools are uniformly supported on Windows. If you are
on Windows, have a look at the later section on Windows development.
+Compiler warning levels
+~~~~~~~~~~~~~~~~~~~~~~~
+
+The ``BUILD_WARNING_LEVEL`` CMake option switches between sets of predetermined
+compiler warning levels that we use for code tidiness. For release builds, the
+default warning level is ``PRODUCTION``, while for debug builds the default is
+``CHECKIN``.
+
+When using ``CHECKIN`` for debug builds, ``-Werror`` is added when using gcc
+and clang, causing build failures for any warning, and ``/WX`` is set with MSVC
+having the same effect.
+
Code Style, Linting, and CI
~~~~~~~~~~~~~~~~~~~~~~~~~~~