Enable compiler warnings. #1792
Conversation
|
Should we provide separate PR into master and then rebase this PR or how did you imagine to go about solving the produced errors? |
|
Someone more daring than me should pick a method. 😁 |
|
Why might want to update the minimum required version to at least 3.15 or work around this bug: https://gitlab.kitware.com/cmake/cmake/-/issues/18317 Otherwise we'll get a lot of complaints about this warning. 😅 |
binary1248
force-pushed
the
feature/compiler_warnings
branch
from
a624063
to
84f69b4
Compare
Should be fixed with the |
|
I think it makes the most sense if people would make PRs onto this branch, otherwise they don't actually see whether they've fixed all the issues. |
binary1248
force-pushed
the
feature/compiler_warnings
branch
from
84f69b4
to
1c03786
Compare
|
Pushed changeset without |
binary1248
force-pushed
the
feature/compiler_warnings
branch
from
1c03786
to
6a9fd59
Compare
|
Added |
| endif() | ||
|
|
||
| foreach(WARNING ${FILE_WARNINGS}) | ||
| set_property(SOURCE ${ARGV} APPEND_STRING PROPERTY COMPILE_FLAGS " ${WARNING}") |
There was a problem hiding this comment.
Could use set_source_files_properties here?
There was a problem hiding this comment.
set_property lets us append to the existing property value in a single line.
| # https://github.com/lefticus/cppbestpractices/blob/master/02-Use_the_Tools_Available.md | ||
|
|
||
| # Helper function to enable compiler warnings for a specific set of files | ||
| function(set_file_warnings) |
There was a problem hiding this comment.
is there a reason we set the options on a set of files rather than on the targets?
There was a problem hiding this comment.
We can select precisely for which files we want to enable our strict warnings (which are treated as errors and cause building to fail). There might be cases where we choose to compile some third party source as part of a target and don't want to enable our warnings for upstream code.
binary1248
force-pushed
the
feature/compiler_warnings
branch
from
6a9fd59
to
7749f3e
Compare
|
Removed |
|
Superseded, integrated and merged in #1846 |
Title says it all.
Currently not expected to build since the enabled warnings are stricter than the current requirements on the code.