Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make warnings errors #70

Merged
merged 10 commits into from Jun 8, 2018
Merged

Conversation

@isaacbrodsky
Copy link
Collaborator

isaacbrodsky commented Jun 4, 2018

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?

Isaac Brodsky
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 4, 2018

Coverage Status

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

Copy link
Contributor

isaachier left a comment

LGTM

@@ -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")

This comment has been minimized.

Copy link
@isaachier

isaachier Jun 4, 2018

Contributor

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

This comment has been minimized.

Copy link
@isaacbrodsky

isaacbrodsky Jun 4, 2018

Author Collaborator

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

This comment has been minimized.

Copy link
@isaachier

isaachier Jun 4, 2018

Contributor

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.

Copy link
Contributor

nrabinowitz left a comment

Seems good to me.

@isaacbrodsky

This comment has been minimized.

Copy link
Collaborator Author

isaacbrodsky commented Jun 5, 2018

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.

@@ -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)

This comment has been minimized.

Copy link
@isaachier

isaachier Jun 5, 2018

Contributor

👍

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

project(h3 VERSION ${H3_VERSION})

set(H3_FLAGS "")

This comment has been minimized.

Copy link
@isaachier

isaachier Jun 5, 2018

Contributor

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

This comment has been minimized.

Copy link
@isaacbrodsky

isaacbrodsky Jun 5, 2018

Author Collaborator

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

Copy link
Contributor

isaachier left a comment

Just one more small change.

# are not correct for MSVC.
string(CONCAT H3_FLAGS ${H3_FLAGS} "-Wall ")

if(CMAKE_BUILD_TYPE STREQUAL "Debug")

This comment has been minimized.

Copy link
@isaachier

isaachier Jun 5, 2018

Contributor

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.

This comment has been minimized.

Copy link
@isaachier

isaachier Jun 5, 2018

Contributor

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

This comment has been minimized.

Copy link
Contributor

isaachier commented Jun 5, 2018

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).

Isaac Brodsky

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

This comment has been minimized.

Copy link
@isaachier

isaachier Jun 5, 2018

Contributor

What about the generator expression?

This comment has been minimized.

Copy link
@isaacbrodsky

isaacbrodsky Jun 5, 2018

Author Collaborator

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

@isaacbrodsky

This comment has been minimized.

Copy link
Collaborator Author

isaacbrodsky commented Jun 5, 2018

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

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)

This comment has been minimized.

Copy link
@isaachier

isaachier Jun 5, 2018

Contributor

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

# 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>)

This comment has been minimized.

Copy link
@isaachier

isaachier Jun 5, 2018

Contributor

👍

This comment has been minimized.

Copy link
@isaachier

isaachier Jun 5, 2018

Contributor

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

This comment has been minimized.

Copy link
@isaacbrodsky

isaacbrodsky Jun 5, 2018

Author Collaborator

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

@@ -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})

This comment has been minimized.

Copy link
@isaachier

isaachier Jun 5, 2018

Contributor

Why is this PUBLIC?

This comment has been minimized.

Copy link
@isaacbrodsky

isaacbrodsky Jun 5, 2018

Author Collaborator

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

@@ -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)

This comment has been minimized.

Copy link
@isaachier

isaachier Jun 5, 2018

Contributor

Typo, should be defaulted on.

Copy link
Contributor

isaachier left a comment

LGTM

@isaacbrodsky isaacbrodsky merged commit 74bccff into uber:master Jun 8, 2018
4 checks passed
4 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
license/cla Contributor License Agreement is signed.
Details
@isaacbrodsky isaacbrodsky deleted the isaacbrodsky:warnings-as-errors branch Jun 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.