Fix compiler warnings
Observe lots of warnings in many packages. This is an umbrella task to fix them. Do not anticipate 100% compliance for the target release. Should be farmed out to developers whenever possible.
#1 Updated by Gianluca Petrillo over 6 years ago
- Status changed from New to Assigned
- Assignee set to Gianluca Petrillo
- Target version changed from v0_02_02 to develop/HEAD
- % Done changed from 0 to 50
- Estimated time set to 4.00 h
- Occurs In v1_01_00 added
- Occurs In deleted (
- Experiment LArSoft added
- Experiment deleted (
#3 Updated by Lynn Garren over 6 years ago
The following CMakeLists.txt files need to be modified to remove the line that tells the build to NOT use Werror:
lardata/RecoObjects/CMakeLists.txt:cet_remove_compiler_flag( CXX -Werror )
larreco/RecoAlg/CMakeLists.txt:cet_remove_compiler_flag( CXX -Werror )
larreco/HitFinder/CMakeLists.txt:cet_remove_compiler_flag( CXX -Werror )
larreco/TrackFinder/CMakeLists.txt:cet_remove_compiler_flag( CXX -Werror )
larreco/ClusterFinder/RStarTree/Makefile:CPPFLAGS = -ggdb -Wall -Werror
larreco/VertexFinder/CMakeLists.txt:cet_remove_compiler_flag( CXX -Werror )
This may uncover more problems.
#4 Updated by Gianluca Petrillo over 6 years ago
I compiled with the
VIGILANT CMake flag level plus
-pedantic -Wformat-y2k -Wswitch-default -Wsync-nand -Wtrampolines -Wlogical-op -Wno-ignored-qualifiers -Wno-error -Wno-overloaded-virtual -Wno-unused-parameter -Wno-switch-default (
PARANOID was unmanageable because of ROOT), then with
CAUTIOUS and removing
- large number of
switch-default which seems unharmful
- many redundant checks (along the line:
unsigned number >= 0): 8 of them commented out
- 1 more redundant check: signalled to the author (might be a mistake)
- 1 more redundant check: signalled to the author (needs some rewriting and some choices)
- 1 infinite loop bug (stops when
unsigned int < 0): fixed
- 1 reference to temporary object stored as class member: fixed (using a static object)
- 2 bugs related to mixing signed and unsigned integers (treating the result as signed): fixed
- 1 constantness bug (returning literal strings as non-const): fixed => small interface change
- 3 copy constructors not constructing their base class: fixed
- 1 memory leak (new structures not deleted): fixed (structures were unnecessary)
- 1 memory leak (sometimes a returned pointer points to a newly created vector): signalled to the author (fixing requires change of semantic)
- 1 unreachable + wrong block of code: commented out
- 1 minor CFLAGS problem (
-Wno-format-y2k makes gcc unhappy): fixed
- 1 apparent bug of inverted boundary check: signalled to the author
At this point, I endorse the introduction of the
The bugs unveiled thanks to it have been at least 5.
I haven't committed the code yet.
#7 Updated by Gianluca Petrillo over 6 years ago
- % Done changed from 80 to 90
I have changed the code to be compilable with
WERROR (halt on any warning) is restored on the whole code.
The code has been compiled without warnings with debug:e4 and prof:e4 quality flags.
It was mostly matter of commenting out unused identifiers, the addition of some dummy Print(Option_t*) methods and changing return types from "const int" to "int" or similar.
Plus a plain bug (a 10x10 array written as it was 11x11).
A few bug fixes entered in the change, which were unveiled by the new warning flags.
For a couple of places, the authors have been contacted and the code I put is just my interpretation of how to fix the bugs in there.
-Wno-unused-variable (probably only a few files do, I did not descend into that) because of problems in CLHEP.
PARANOID level is not that far, neither, but it would require
-Wno-cast-qual because of ROOT (easy to solve... in ROOT) and to solve a number of
Keeping the bug open for a while while waiting for the answers from the authors and waiting general feedback. But as much as I can tell, warnings are gone.
Note to the developers: compilation of code which was not committed yet can now fail due to the stricter compilation flags. That almost always means that the code does need to be fixed.
#8 Updated by Gianluca Petrillo over 6 years ago
- % Done changed from 90 to 80
After talking with Rick, I have restored the old compiler flags (
CAUTIOUS and selected
The new flags could "break" the compilation of existing code, and it's better if the users are aware of that.
I actually endorse going to
VIGILANT immediately but qualifying the new flags as
-Wno-error=flag for a while to give the users the time to think of fixes.
The new code does break a bit of the code: classes inherited from
geo::Geometry which overload
View methods will have to remove the
const qualifier from the return type (that is true also for other classes, but I haven't seen them used as base classes).