Make warnings errors #70

isaacbrodsky · 2018-06-04T18:32:49Z

I think we're already at or near 0 warnings, so we could make warnings errors to preserve that. It's also possible some of the compilers in CI have warnings which we don't notice when developing.

Thoughts?

coveralls · 2018-06-04T18:45:53Z

Coverage remained the same at 100.0% when pulling 08e4f1a on isaacbrodsky:warnings-as-errors into abbc693 on uber:master.

isaachier

LGTM

isaachier · 2018-06-04T19:24:29Z

cmake/toolchain.cmake

@@ -18,7 +18,7 @@ set(CMAKE_C_STANDARD 11)
 if(NOT WIN32)
    # Compiler options are set only on non-Windows, since these options
    # are not correct for MSVC.
-    set(CMAKE_C_FLAGS_INIT "-Wall")
+    set(CMAKE_C_FLAGS_INIT "-Wall -Werror")
    string(CONCAT CMAKE_C_FLAGS_DEBUG_INIT
           "-g -gdwarf-2 -g3 -O0 -fno-inline -fno-eliminate-unused-debug-types "
           "--coverage")


Totally unrelated question, where did these flags come from? They look pretty specialized. Why not use defaults and append --coverage to that?

At least a few of these were added by @dfellis in #8 (1bfa863). Not sure about some of the others.

OK thanks. At some point it might be worth revisiting the flags here. I am not sure if they are the best options and if there is a better way to use the toolchain file.

nrabinowitz

Seems good to me.

isaacbrodsky · 2018-06-05T17:18:06Z

Made an update to this PR, warnings as errors are still defaulted on, but also has an option to disable.

edit: and once more for moving the option out of the toolchain file.

isaachier · 2018-06-05T17:56:22Z

CMakeLists.txt

@@ -47,6 +47,14 @@ set(H3_SOVERSION 1)

 project(h3 VERSION ${H3_VERSION})

+set(H3_FLAGS "")
+if(NOT WIN32)
+    option(WARNINGS_AS_ERRORS "Warnings are treated as errors" ON)


isaachier · 2018-06-05T17:56:57Z

CMakeLists.txt

@@ -47,6 +47,14 @@ set(H3_SOVERSION 1)

 project(h3 VERSION ${H3_VERSION})

+set(H3_FLAGS "")


Where are the other flags? I'd remove them from cmake/toolchain.cmake and put them here instead.

Seems reasonable. I've moved the other flags here.

isaachier

Just one more small change.

isaachier · 2018-06-05T18:36:26Z

CMakeLists.txt

+    # are not correct for MSVC.
+    string(CONCAT H3_FLAGS ${H3_FLAGS} "-Wall ")
+
+    if(CMAKE_BUILD_TYPE STREQUAL "Debug")


So I once thought this was a good idea too. Turns out, checking for "Debug" build might be done at build time instead of configuration time, so you need to use a generator expression here. Something like this should do it:

string(CONCAT H3_FLAGS "$<$<CONFIG:Debug>:-g -gdwarf-2 -g3 -O0 -fno-inline -fno-eliminate-unused-debug-types> ")

More on this topic here: https://stackoverflow.com/a/24470998/1930331.

For the record, I'm not convinced it's a good idea to use -O0 or -g in these options because CMake will add them as the default debug flags.

To be clear, please remove the if statement entirely and you can append this generator expression to the earlier -Wall if you'd like.

isaachier · 2018-06-05T18:43:28Z

I see some of the tests are failing. I've had this issue too, and it has to do with target_compile_options expecting a list instead of a string. This is actually a good thing because now you no longer have to worry about spaces and string concatenation. Just unquote the options above so everything works (for generator expression, just do $<$<CONFIG:Debug>:...> with no wrapping quotes).

isaachier · 2018-06-05T19:07:35Z

CMakeLists.txt

+
+    if(CMAKE_BUILD_TYPE STREQUAL "Debug")
+        list(APPEND H3_COMPILE_FLAGS
+                -g -gdwarf-2 -g3 -O0 -fno-inline -fno-eliminate-unused-debug-types)


What about the generator expression?

Haven't been pushed yet, still running some tests locally

isaacbrodsky · 2018-06-05T19:07:58Z

There was also a second issue that --coverage was no longer passed to the linker, which required adding an additional option there.

isaachier · 2018-06-05T19:13:30Z

CMakeLists.txt

+            list(APPEND H3_COMPILE_FLAGS --coverage)
+            # --coverage is not passed to the linker, so this option is needed
+            # to fully enable coverage.
+            list(APPEND H3_LINK_FLAGS -fprofile-arcs)


I think --coverage would work here too (at least for GCC and clang). But you are right for adding the linker flags in general.

isaachier · 2018-06-05T19:13:57Z

CMakeLists.txt

+    # are not correct for MSVC.
+    list(APPEND H3_COMPILE_FLAGS -Wall)
+
+    list(APPEND H3_COMPILE_FLAGS $<$<CONFIG:Debug>:-g -gdwarf-2 -g3 -O0 -fno-inline -fno-eliminate-unused-debug-types>)


Please remove -O0 and -g. They don't serve the same purpose here as in the toolchain file.

I see -g is in the default flags for Debug, but I don't see -O0. What is the reason for removing that option?

isaachier · 2018-06-05T19:14:21Z

CMakeLists.txt

@@ -147,6 +171,9 @@ set(ALL_SOURCE_FILES
 # Build the H3 library
 add_library(h3 ${LIB_SOURCE_FILES})

+target_compile_options(h3 PRIVATE ${H3_COMPILE_FLAGS})
+target_link_libraries(h3 PUBLIC ${H3_LINK_FLAGS})


Why is this PUBLIC?

I was thinking of writing this in a different way but didn't update, will fix it.

isaachier · 2018-06-05T19:15:47Z

CHANGELOG.md

@@ -10,6 +10,7 @@ The public API of this library consists of the functions declared in file
 - Generator for the faceCenterPoint table (#67)
 ### Changed
 - Moved Vec3d structure to `vec3d.h` (#67)
+- Added CMake `WARNINGS_AS_ERRORS` option, defauled on, for Clang and GCC (#70)


Typo, should be defaulted on.

isaachier

LGTM

Make warnings errors

Make warnings errors

f0b9c59

isaachier approved these changes Jun 4, 2018

View reviewed changes

nrabinowitz approved these changes Jun 4, 2018

View reviewed changes

Isaac Brodsky added 3 commits June 4, 2018 17:24

Merge branch 'master' into warnings-as-errors

97c91c5

Add CHANGELOG for warnings as errors

1ea115c

Add WARNINGS_AS_ERRORS option

e2d3ff4

Move WARNINGS_AS_ERRORS out of toolchain file to target properties

1f9cefc

isaachier suggested changes Jun 5, 2018

View reviewed changes

Move all compiler options to target_compile_options

6ffbd46

isaachier suggested changes Jun 5, 2018

View reviewed changes

target_compile_options as list

e219feb

isaachier reviewed Jun 5, 2018

View reviewed changes

Switch to generator expressions for Debug builds

4659288

isaachier reviewed Jun 5, 2018

View reviewed changes

Switch to --coverage for all coverage options

08e4f1a

isaachier reviewed Jun 5, 2018

View reviewed changes

Remove unnecessary -g compile flag. Fix CHANGELOG typo.

52a5c9f

isaachier approved these changes Jun 5, 2018

View reviewed changes

nrabinowitz approved these changes Jun 7, 2018

View reviewed changes

isaacbrodsky merged commit 74bccff into uber:master Jun 8, 2018

isaacbrodsky deleted the warnings-as-errors branch June 8, 2018 16:19

mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022

Merge pull request uber#70 from isaacbrodsky/warnings-as-errors

f69d141

Make warnings errors

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make warnings errors #70

Make warnings errors #70

		@@ -47,6 +47,14 @@ set(H3_SOVERSION 1)

		project(h3 VERSION ${H3_VERSION})

		set(H3_FLAGS "")

Make warnings errors #70

Make warnings errors #70

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment