ARROW-1615 Added BUILD_WARNING_LEVEL and BUILD_WARNING_FLAGS to Setup… #1145

renesugar · 2017-09-28T18:37:41Z

…CxxFlags.cmake

wesm

Thanks, this will be really useful. I've been managing these flags manually. One question I have is the terminology, you have

CHECKIN
EVERYTHING
DEVELOPMENT

It seems like CHECKIN is really DEVELOPMENT, and what's now DEVELOPMENT is actually production

wesm · 2017-09-28T23:15:59Z

cpp/cmake_modules/SetupCxxFlags.cmake

+# Build warning level (CHECKIN, EVERYTHING, etc.)
+
+# GCC/Clang warning flags by version:
+# https://github.com/Barro/compiler-warnings


What is the reference for? This repo has a GPL license in it, so want to make sure no IP from that codebase is being taken in here

I just included as a way for people to see which build of Clang introduced a warning; I'll remove it. I came up with the list that I use from compiling with -Weverything and suppressing warnings until the existing code base built with the version of the Clang compiler that I have.

CHECKIN adds "-Werror" and a few other warnings like the Travis CI build.

Currently, warnings are not treated as errors when building on your own development machine. Then, you check in your code and Travis CI etc. add "-Werror" and a few more warnings.

I'll change "CHECKIN" to "PRODUCTION".

"DEVELOPMENT" does not treat warnings as errors.

Sorry, I miscommunicated. We should not fail users' builds because of compiler warnings. But developers, yes. So

Default: PRODUCTION (warnings to not cause errors)
Development: CHECKIN (warnings -> errors)
Pedantic: EVERYTHING

You can leave the reference to that repo as long as it is clear that no code is being redistributed

wesm · 2017-09-28T23:17:16Z

cpp/cmake_modules/SetupCxxFlags.cmake

+    set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /WX")
+  elseif ("${COMPILER_FAMILY}" STREQUAL "clang")
+    # NOTE: -Wconversion does not return the same warnings on CLang and GNU (build can still fail even when enabled)
+    set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -Weverything -Wno-c++98-compat -Wno-c++98-compat-pedantic -Wno-deprecated -Wno-weak-vtables -Wno-padded -Wno-unused-parameter -Wno-undef -Wno-documentation-deprecated-sync -Wno-reserved-id-macro -Wno-shadow -Wno-switch-enum -Wno-documentation -Wno-exit-time-destructors -Wno-global-constructors -Wno-weak-template-vtables -Wno-undefined-reinterpret-cast -Wno-implicit-fallthrough -Wno-old-style-cast -Wno-unreachable-code-return -Wno-float-equal -Wno-missing-prototypes -Wno-double-promotion -Wno-non-virtual-dtor -Wno-unused-macros -Wno-covered-switch-default -Wno-unreachable-code-break -Wno-extra-semi -Wno-range-loop-analysis -Wno-shift-sign-overflow -Wno-used-but-marked-unused -Wno-missing-variable-declarations -Wno-gnu-zero-variadic-macro-arguments -Wconversion -Wno-sign-conversion -Wc++11-narrowing -Wnarrowing")


Any way to make this line more readable?

wesm · 2017-09-28T23:20:17Z

cpp/cmake_modules/SetupCxxFlags.cmake

+    string(REPLACE "/W3" "" CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS}")
+    set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /W3")
+  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 -Wno-unused-parameter -Wno-undef -Wno-documentation-deprecated-sync -Wno-reserved-id-macro -Wno-shadow -Wno-switch-enum -Wno-documentation -Wno-exit-time-destructors -Wno-global-constructors -Wno-weak-template-vtables -Wno-undefined-reinterpret-cast -Wno-implicit-fallthrough -Wno-old-style-cast -Wno-unreachable-code-return -Wno-float-equal -Wno-missing-prototypes -Wno-double-promotion -Wno-non-virtual-dtor -Wno-unused-macros -Wno-covered-switch-default -Wno-unreachable-code-break -Wno-extra-semi -Wno-range-loop-analysis -Wno-shift-sign-overflow -Wno-used-but-marked-unused -Wno-missing-variable-declarations -Wno-gnu-zero-variadic-macro-arguments -Wconversion -Wno-sign-conversion -Wc++11-narrowing -Wnarrowing")


same comment -- I think there are line continuations in newer CMake

Added line continuations. CMake adds extra spaces in the output when I indent the lines.

wesm · 2017-09-28T23:20:42Z

cpp/cmake_modules/SetupCxxFlags.cmake

+if (NOT ("${COMPILER_FAMILY}" STREQUAL "msvc"))
+set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} -std=c++11")
+endif()
+message( STATUS "APPLE: " ${APPLE} )


Yes; removed.

…entries

wesm · 2017-09-29T14:20:09Z

Looks like -Wcast-align may be the last warning to make the build happy

wesm · 2017-09-29T16:40:10Z

cpp/cmake_modules/SetupCxxFlags.cmake

+    -Wno-extra-semi -Wno-cast-align -Wno-shift-sign-overflow \
+    -Wno-used-but-marked-unused -Wno-missing-variable-declarations \
+    -Wno-gnu-zero-variadic-macro-arguments -Wconversion -Wno-sign-conversion \
+    -Wno-disabled-macro-expansion -Wc++11-narrowing -Wnarrowing -Wno-shorten-64-to-32")


If we are not passing -Werror, do we need all these suppressions in non-dev / checkin builds? A lot of these are pedantic warnings so it might be better to trim this down to a smaller list

(i.e. not using -Weverything in prod / non-dev builds)

I'll change that path back to "-Wall".

I changed the production path back to "-Wall". Travis will add warnings to "-Wall" again.

Some warning suppressions subtracting from "-Weverything" were added for other versions of Clang used by Travis etc.. If Arrow developers are using newer versions of Clang, some of the warning suppressions can be removed; others require code changes.

BUILD_WARNING_LEVEL Values:

Checkin (use before checkin to attempt to determine if Travis CI build will fail)

This warning set subtracts from -Weverything by suppressing specific warning flags that prevent the current source code from building with various versions of Clang and adds warnings used by Travis, etc.

These are warnings that have never been enabled (Clang adds new warnings to -Weverthing ; -Wall is the list in common with gcc).

cmake -DBUILD_WARNING_LEVEL=checkin ..

Everything (use with the Clang compiler's -Weverything flag to find potential errors)

Use with BUILD_WARNING_FLAGS to disable warnings to get to the next warning.

http://amattn.com/p/better_apps_clang_weverything_or_wall_is_a_lie.html

cmake -DBUILD_WARNING_LEVEL=everything ..
make VERBOSE=1 unittest
cmake -DBUILD_WARNING_LEVEL=everything -DBUILD_WARNING_FLAGS="-Wno-padded -Wno-sign-conversion" ..
make VERBOSE=1 unittest
-DBUILD_WARNING_LEVEL=everything -DBUILD_WARNING_FLAGS="-Wno-padded -Wno-sign-conversion -Wno-unused-parameter" ..
make VERBOSE=1 unittest
etc.

Production (default; warnings are not treated as errors)

This warning set uses "-Wall". This is the default warning set to which Travis adds extra warnings and used by anyone compiling Arrow without specifying -DBUILD_WARNING_LEVEL when running cmake.

cmake ..

wesm · 2017-09-29T16:42:56Z

cpp/cmake_modules/SetupCxxFlags.cmake

+    -Wno-extra-semi -Wno-cast-align -Wno-shift-sign-overflow \
+    -Wno-used-but-marked-unused -Wno-missing-variable-declarations \
+    -Wno-gnu-zero-variadic-macro-arguments -Wconversion -Wno-sign-conversion \
+    -Wno-disabled-macro-expansion -Wc++11-narrowing -Wnarrowing -Wno-shorten-64-to-32")


Some of these "pedantic" diagnostics seem useful:

non-virtual-dtor

sign-conversion

unused-parameter (this should probably be a separate patch to clean up unused params)

These selections allow the current code to build.

"-Weverything" in Clang enables many more warnings that would otherwise not be seen in Clang or Gcc using other warning flags.

To get the current code to build with fewer warnings suppressed, developers can enable individual diagnostics and remove them from the list when the code is modified (suppress the warning where it occurs if the code does not need to be fixed or change the code so the warning is not issued).

…to -Wall

wesm

+1, thank you for this! We'll want to document this in the cpp/README.md, but I can do that in a follow up patch. I will take a look at disabling some of the warning suppressions to fix some of our bad code smells

wesm · 2017-09-29T20:42:16Z

This build is failing for me locally, I'll submit a patch

…CxxFlags.cmake Author: Rene Sugar <[email protected]> Closes apache#1145 from renesugar/ARROW-1615 and squashes the following commits: 71a615e [Rene Sugar] ARROW-1615 Add -Wno-vla-extension and change non-checkin builds back to -Wall 1895843 [Rene Sugar] ARROW-1615 Add -Wno-cast-align 5fe4e8e [Rene Sugar] ARROW-1615 Move -Wno-shorten-64-to-32 after -Wconversion 9d3c7ec [Rene Sugar] ARROW-1615 Identify compiler version for clang-802 plus more warning entries 5ebaf86 [Rene Sugar] ARROW-1615 Moved version specific warning entry 971e61a [Rene Sugar] ARROW-1615 Fixed version specific warning entry 6cf2497 [Rene Sugar] ARROW-1615 Added more version specific Clang warning entries 50def43 [Rene Sugar] ARROW-1615 Updated build warning level terminology ea906eb [Rene Sugar] ARROW-1615 Check compiler version before disabling some warnings 159e189 [Rene Sugar] ARROW-1615 Include CompilerInfo before SetupCxxFlags in arrow/python 8359c96 [Rene Sugar] ARROW-1615 Added BUILD_WARNING_LEVEL and BUILD_WARNING_FLAGS to SetupCxxFlags.cmake

ARROW-1615 Added BUILD_WARNING_LEVEL and BUILD_WARNING_FLAGS to Setup…

8359c96

…CxxFlags.cmake

renesugar closed this Sep 28, 2017

renesugar reopened this Sep 28, 2017

ARROW-1615 Include CompilerInfo before SetupCxxFlags in arrow/python

159e189

wesm reviewed Sep 28, 2017

View reviewed changes

renesugar added 7 commits September 28, 2017 17:23

ARROW-1615 Check compiler version before disabling some warnings

ea906eb

ARROW-1615 Updated build warning level terminology

50def43

ARROW-1615 Added more version specific Clang warning entries

6cf2497

ARROW-1615 Fixed version specific warning entry

971e61a

ARROW-1615 Moved version specific warning entry

5ebaf86

ARROW-1615 Identify compiler version for clang-802 plus more warning …

9d3c7ec

…entries

ARROW-1615 Move -Wno-shorten-64-to-32 after -Wconversion

5fe4e8e

ARROW-1615 Add -Wno-cast-align

1895843

wesm reviewed Sep 29, 2017

View reviewed changes

ARROW-1615 Add -Wno-vla-extension and change non-checkin builds back …

71a615e

…to -Wall

wesm approved these changes Sep 29, 2017

View reviewed changes

asfgit closed this in d4e09c7 Sep 29, 2017

wesm deleted the ARROW-1615 branch September 29, 2017 20:30

ARROW-1615 Added BUILD_WARNING_LEVEL and BUILD_WARNING_FLAGS to Setup… #1145

ARROW-1615 Added BUILD_WARNING_LEVEL and BUILD_WARNING_FLAGS to Setup… #1145

Uh oh!

Conversation

renesugar commented Sep 28, 2017

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Sep 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

wesm commented Sep 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants