Bug #5105
Fix compiler warnings
100%
Description
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.
History
#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 (
v0_02_00) - Experiment LArSoft added
- Experiment deleted (
-)
#2 Updated by Gianluca Petrillo over 6 years ago
- % Done changed from 50 to 80
Only one warning standing (Wesley Ketchum is fixing it).
#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 -Wno-error everywhere.
Found:
- large number of overloaded-virtual, unused-parameter and 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 without -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 -Wtype-limits warning.
The bugs unveiled thanks to it have been at least 5.
I haven't committed the code yet.
#5 Updated by Erica Snider over 6 years ago
Hi Gianluca,
Great list! You might also think about contacting the author for the unreachable code. He/she must have had something in mind with that block.
Erica
#6 Updated by Lynn Garren over 6 years ago
This is great! Does this mean that we can use VIGILANT for most of larsoft? Or do you think larsoft should continue to use CAUTIOUS and add -Wtype-limits?
I see no reason to delay pushing the changes. They will get picked up by the next nightly update.
#7 Updated by Gianluca Petrillo over 6 years ago
- % Done changed from 80 to 90
I have changed the code to be compilable with VIGILANT. 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.
The package larsim needs -Wno-unused-variable (probably only a few files do, I did not descend into that) because of problems in CLHEP.
The 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 shadow and switch-default.
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 -Wno-error).
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 SignalType or 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).
#9 Updated by Lynn Garren over 6 years ago
- Status changed from Assigned to Resolved
- % Done changed from 80 to 100
- Occurs In v1_00_01 added
- Occurs In deleted (
v1_01_00)
Compiler warnings are fixed in larsoft v1_00_02.
#10 Updated by Gianluca Petrillo about 6 years ago
- Target version changed from develop/HEAD to v1_01_00
#11 Updated by Gianluca Petrillo about 6 years ago
- Status changed from Resolved to Closed