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.

Sign up for GitHub

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

Jump to bottom

Add KIVA_WERROR option to interpret compiler warnings as errors #55

Merged
merged 12 commits into from
Aug 31, 2022
Merged

Add KIVA_WERROR option to interpret compiler warnings as errors #55

merged 12 commits into from
Aug 31, 2022

Conversation

galanCA
Copy link
Contributor

@galanCA galanCA commented Aug 18, 2022

Description

The goal is to have the option to turn warnings into errors by

Major changes:

  • Add compiler-flag.cmake and an interface library to distribute the compiler flags
  • Add the option to turn off and on Werror (/WX for windows).

@galanCA galanCA self-assigned this Aug 18, 2022
@galanCA
Copy link
Contributor Author

galanCA commented Aug 18, 2022 •
edited
Loading

@nealkruis

  • What should the default option for KIVA_WERROR be?
  • Would you like the CI to have Werror ON or OFF?

@nealkruis
Copy link
Member

I go back and forth on the default. I think I want it on by default, and then executables linking to Kiva can decide if they want to turn it off.

Definitely needs to be on for the CI, otherwise it won't keep us from merging warnings.

@codecov
Copy link

codecov bot commented Aug 18, 2022 •
edited
Loading

Codecov Report

Merging #55 (6ba01e5) into develop (5a14aa0) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop      #55   +/-   ##
========================================
  Coverage    59.19%   59.19%           
========================================
  Files           21       21           
  Lines         5065     5065           
========================================
  Hits          2998     2998           
  Misses        2067     2067
Flag Coverage Δ
integration 59.19% <ø> (ø)
unit 59.19% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@galanCA galanCA changed the title Add werror option Adds KIVA_WERROR option Aug 19, 2022
@galanCA galanCA requested a review from nealkruis August 19, 2022 15:42
nealkruis
nealkruis requested changes Aug 19, 2022
View reviewed changes
cmake/compiler-flags.cmake Show resolved Hide resolved
CMakeLists.txt Outdated
@@ -8,6 +8,7 @@ option(ENABLE_OPENMP "Use OpenMP" OFF)
option( KIVA_GROUND_PLOT "Build ground plotting library" ON )
mark_as_advanced(FORCE BUILD_GROUND_PLOT)
option( KIVA_TESTING "Build tests" ON )
option( KIVA_WERROR "Turn on warnings into error" ON )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed my mind on this one. Let's make it default to OFF, and handle things similarly to the fmt repository.

nealkruis
nealkruis reviewed Aug 19, 2022
View reviewed changes
cmake/compiler-flags.cmake Outdated
Comment on lines 14 to 30
$<$<CXX_COMPILER_ID:GNU>:
-Wall
-Wextra
-Wpedantic
$<$<BOOL:${KIVA_WERROR}>:
-Werror # Turn warnings into errors
>
>

$<$<CXX_COMPILER_ID:Clang>:
-Wall
-Wextra
-Wpedantic
$<$<BOOL:${KIVA_WERROR}>:
-Werror # Turn warnings into errors
>
>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is WET.

@galanCA
Copy link
Contributor Author

galanCA commented Aug 25, 2022 •
edited
Loading

@nealkruis I tested that the interface library was only applied to the corresponding target by:

  • Creating a new branch in CSE where:
    • I checkout this branch for kiva
    • make sure to turn on the WX flag in Kiva
    • and turn CSE warning flag from W3 to W4
      From this, I would expect the CSE warnings to turn into errors if the WX flags were passed to CSE. But I was able to see the warnings and no errors allowing CSE to build successfully.

I went back and added the ability to turn on KIVA_WERROR option inside vendor/cmakelist.txt and check the CMakeCache.txt. But the output was the same as before, I could see the warnings, no errors and CSE build successfully. From this, I derived that the interface library is only affecting the private target and not the dependencies of the target.

Do you agree with this conclusion?

@galanCA galanCA requested a review from nealkruis August 26, 2022 16:34
nealkruis
nealkruis requested changes Aug 30, 2022
View reviewed changes
Copy link
Member

@nealkruis nealkruis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting very close. I left two comments, but I think we are also missing a link to the common interface on one of the targets.

src/libgroundplot/CMakeLists.txt Outdated
@@ -5,16 +5,10 @@ set(groundplot_src GroundPlot.cpp

add_library(groundplot STATIC ${groundplot_src})

target_link_libraries(groundplot fmt mgl-static libkiva)
target_link_libraries(groundplot fmt mgl-static libkiva kiva_common_interface)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No keyword?

src/kiva/CMakeLists.txt Outdated
@@ -12,16 +12,10 @@ set(CMAKE_INSTALL_RPATH "@executable_path")

add_executable(kiva ${kiva_src})

target_link_libraries(kiva libkiva groundplot boost_program_options yaml-cpp)
target_link_libraries(kiva libkiva groundplot boost_program_options yaml-cpp kiva_common_interface)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No keyword?

@nealkruis nealkruis changed the title Adds KIVA_WERROR option Add KIVA_WERROR option to interpret compiler warnings as errors Aug 30, 2022
@nealkruis nealkruis merged commit 4ab6361 into develop Aug 31, 2022
@nealkruis nealkruis deleted the add-werror-option branch August 31, 2022 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nealkruis nealkruis nealkruis approved these changes

Assignees

@galanCA galanCA

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@galanCA @nealkruis