Project

General

Profile

Bug #5105

Fix compiler warnings

Added by Erica Snider almost 6 years ago. Updated over 5 years ago.

Status:
Closed
Priority:
Normal
Category:
-
Target version:
Start date:
12/16/2013
Due date:
% Done:

100%

Estimated time:
4.00 h
Occurs In:
Experiment:
LArSoft
Co-Assignees:
Duration:

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 almost 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 almost 6 years ago

  • % Done changed from 50 to 80

Only one warning standing (Wesley Ketchum is fixing it).

#3 Updated by Lynn Garren almost 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 almost 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 almost 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 almost 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 almost 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 almost 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 almost 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 over 5 years ago

  • Target version changed from develop/HEAD to v1_01_00

#11 Updated by Gianluca Petrillo over 5 years ago

  • Status changed from Resolved to Closed


Also available in: Atom PDF