-
Notifications
You must be signed in to change notification settings - Fork 948
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
Fix compiler warnings and fail on warnings in future #2687
Conversation
@@ -25,6 +25,7 @@ jobs: | |||
- IMAGE: melodic-ci | |||
CATKIN_LINT: true | |||
env: | |||
CXXFLAGS: "-Werror -Wall -Wextra -Wwrite-strings -Wunreachable-code -Wpointer-arith -Wredundant-decls -Wno-deprecated-copy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't we want -pedantic -std=c++14
as well?
Also I would have expected issues with upstream library headers...
Are all potentially-problematic ones included as -isystem
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We never used -pedantic
in the past and as our target compilers are gcc + clang, I personally don't mind allowing non-standard compiler extensions.
Codecov Report
@@ Coverage Diff @@
## master #2687 +/- ##
==========================================
+ Coverage 60.55% 60.56% +0.01%
==========================================
Files 402 402
Lines 29601 29601
==========================================
+ Hits 17922 17924 +2
+ Misses 11679 11677 -2
Continue to review full report at Codecov.
|
We never used `-pedantic` in the past
I believe so, but we also never compiled with `-Werror`.
I see `-pedantic` simply as an extension from "warning-free" to "warning-free according to specified standard" here.
and as our target compilers are gcc + clang,
I agree, but some users provide fixes for MS Visual Studio as well (see upcoming maintainer meeting).
(Not that MSVS would be standard conform...)
I personally don't mind allowing non-standard compiler extensions.
We [explicitly disable extension support in our compile instructions](https://github.com/ros-planning/moveit/blob/master/moveit_core/CMakeLists.txt#L7-L8)
and I see no reason not to actually enforce this on CI (unless huge chunks would start to fail).
|
We didn't do this explicitly but implicitly: Travis CI had a WARNINGS_OK=false variable, which made the build fail on any warnings (not only compiler warnings).
We can give it a try, yes. |
No description provided.