fix all compiler warnings for AnalysisBase on latest MacOS

New compiler, new set of warnings...

The main theme here is that clang now checks for range-based for loops whether it introduces unneeded temporaries, i.e. if the user asks for a reference for the loop variable, but the iterator doesn't return a reference or a reference of a different type, or conversely if the user asked for const auto, but could have been const auto&.

In practice that usually hits us as:

for (const auto& jet : jetContainer)

which should be:

for (const auto *jet : jetContainer)

Also a couple of places like this (which miss the const qualifier on the first template parameter):

std::map<std::string,std::string> stringMap;
for (const std::pair<std::string,std::string>& stringPair : stringMap)

For the most part I just replaced the loop variable type with what seemed correct. In a few places I put explicit comments as to why I chose the type.

Also a fair number of warnings for unused member variables in various packages. Since I am not an expert on any of these packages and this can point to an actual bug, I commented out all unused variables and added a comment for actual experts to check and remove.

I removed a couple of checks for this != nullptr which were originally introduced to check whether the user called a member function via a null pointer. However those checks are assert-based, so with cmake they won't be included when the user calls them from a release, making this check mostly useless.

There some other warnings I fixed in the process that should hopefully be self-explanatory.

added 18 commits

• 17c5633c...ce1f96cc - 17 commits from branch atlas:master
• 64fd197e - fix all compiler warnings for AnalysisBase on latest MacOS

Compare with previous version

WARNING: big files (>100K) are found in the changeset

328K in file PhysicsAnalysis/TopPhys/xAOD/TopAnalysis/Root/EventSaverFlatNtuple.cxx

This merge request affects 35 packages. Since this is a long list, it will not be printed.

Adding @cdelitzs ,@bmondal ,@spalazzo ,@dshope ,@martindl ,@krumnack ,@jveatch ,@xiaozhon ,@fsforza ,@mleblanc ,@jojungge ,@oducu ,@akraszna ,@meehan ,@mvanadia ,@tdado ,@maklein ,@yoyamagu ,@goetz ,@markowen ,@amorley ,@nkoehler ,@sroe ,@nakahama ,@szambito ,@omajersk ,@gartoni ,@sschramm ,@rnewhous ,@ssnyder ,@rbianchi ,@adbailey as watchers

added Analysis AnalysisTop Core EDM Egamma InnerDetector JetEtmiss Reconstruction Tau Trigger master review-pending-level-1 labels

CI Result SUCCESS (hash 64fd197e)

	Athena	AthSimulation	AthGeneration	AnalysisBase	AthAnalysis
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view
Athena: number of compilation errors 0, warnings 4
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 2
AnalysisBase: number of compilation errors 0, warnings 3
AthAnalysis: number of compilation errors 0, warnings 2
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 25881]

added 1 commit

• 758a14de - fix all compiler warnings for AnalysisBase on latest MacOS

Compare with previous version

WARNING: big files (>100K) are found in the changeset

328K in file PhysicsAnalysis/TopPhys/xAOD/TopAnalysis/Root/EventSaverFlatNtuple.cxx

This merge request affects 35 packages. Since this is a long list, it will not be printed.

Adding @cdelitzs ,@bmondal ,@spalazzo ,@dshope ,@martindl ,@krumnack ,@jveatch ,@xiaozhon ,@fsforza ,@mleblanc ,@jojungge ,@oducu ,@akraszna ,@meehan ,@mvanadia ,@tdado ,@maklein ,@yoyamagu ,@goetz ,@markowen ,@amorley ,@nkoehler ,@sroe ,@nakahama ,@szambito ,@omajersk ,@gartoni ,@sschramm ,@rnewhous ,@ssnyder ,@rbianchi ,@adbailey as watchers

I would've preferred to take it apart into multiple commits. And multiple merge requests even. A merge request that touches so many files/packages at the same time will just be a lot more effort to review than 10 smaller ones.

Also, what about this warning?

# CMake ---> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DAsgMessagingLib_EXPORTS -DHAVE_64_BITS -DPACKAGE_VERSION="AsgMessaging-00-00-00" -DPACKAGE_VERSION_UQ=AsgMessaging-00-00-00 -DROOTCORE -DXAOD_ANALYSIS -DXAOD_STANDALONE -D__IDENTIFIER_64BIT__ -I/Users/krasznaa/ATLAS/sw/projects/analysisbase/athena/Control/AthToolSupport/AsgMessaging -I/Users/krasznaa/ATLAS/sw/projects/analysisbase/athena/Control/CxxUtils -isystem /Users/krasznaa/ATLAS/sw/projects/analysisbase/build/install/AnalysisBaseExternals/22.2.6/InstallArea/x86_64-mac1015-clang12-opt/include -DNDEBUG -O2 -Wall -Wno-long-long -Wno-deprecated -Wno-unused-local-typedefs -Wwrite-strings -Wpointer-arith -Woverloaded-virtual -Wextra -Werror=return-type -pedantic -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk -mmacosx-version-min=10.15 -fPIC -std=c++17 -o CMakeFiles/AsgMessagingLib.dir/Root/MessagePrinterOverlay.cxx.o -c /Users/krasznaa/ATLAS/sw/projects/analysisbase/athena/Control/AthToolSupport/AsgMessaging/Root/MessagePrinterOverlay.cxx
In file included from /Users/krasznaa/ATLAS/sw/projects/analysisbase/athena/Control/AthToolSupport/AsgMessaging/Root/MessagePrinterOverlay.cxx:13:
/Users/krasznaa/ATLAS/sw/projects/analysisbase/athena/Control/AthToolSupport/AsgMessaging/AsgMessaging/MessagePrinterOverlay.h:77:22: warning: private field 'm_current' is not used [-Wunused-private-field]
    IMessagePrinter *m_current = nullptr;
                     ^
1 warning generated.

I have not looked at the code, I just have a list of packages with warnings myself as well, and I noticed that AsgMessaging was not in your list of fixes.

[bash][wifibridge]:BuildLogs > grep -l "warning:" *
AsgExampleTools.log
AsgMessaging.log
AthContainers.log
BoostedJetTaggers.log
CutBookkeeperUtils.log
CxxUtils.log
DiTauMassTools.log
GammaORTools.log
HFORTools.log
IsolationSelection.log
IsolationTool.log
JetJvtEfficiency.log
JetMomentTools.log
JetRec.log
JetReclustering.log
JetUncertainties.log
METUtilities.log
MuonEfficiencyCorrections.log
PMGTools.log
PhotonEfficiencyCorrection.log
PhotonVertexSelection.log
TopAnalysis.log
TopCorrections.log
TopEventReconstructionTools.log
TopJetSubstructure.log
TopObjectSelectionTools.log
TopParticleLevel.log
TopPartons.log
TopSystematicObjectMaker.log
TrackVertexAssociationTool.log
tauRecTools.log
xAODCaloRings.log
xAODCore.log
xAODMissingET.log
xAODTracking.log
[bash][wifibridge]:BuildLogs >

CI Result FAILURE (hash 758a14de)

	Athena	AthSimulation	AthGeneration	AnalysisBase	AthAnalysis
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 25899]

added 1 commit

• 6fe209d1 - replace assert() statement with if() statement to force checking

Compare with previous version

WARNING: big files (>100K) are found in the changeset

328K in file PhysicsAnalysis/TopPhys/xAOD/TopAnalysis/Root/EventSaverFlatNtuple.cxx

This merge request affects 36 packages. Since this is a long list, it will not be printed.

Adding @jchapman ,@bmondal ,@spalazzo ,@dshope ,@martindl ,@krumnack ,@jveatch ,@xiaozhon ,@fsforza ,@mleblanc ,@jojungge ,@markowen ,@oducu ,@ahaas ,@tdado ,@maklein ,@yoyamagu ,@adbailey ,@goetz ,@meehan ,@amorley ,@rnewhous ,@cdelitzs ,@nkoehler ,@tkharlam ,@sroe ,@nakahama ,@szambito ,@omajersk ,@ssnyder ,@akraszna ,@sschramm ,@tadej ,@gartoni ,@rbianchi ,@mvanadia as watchers

added Overlay label

CI Result FAILURE (hash 6fe209d1)

	Athena	AthSimulation	AthGeneration	AnalysisBase	AthAnalysis
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view
Athena: number of compilation errors 0, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 25930]

Jenkins please retry a build

WARNING: big files (>100K) are found in the changeset

328K in file PhysicsAnalysis/TopPhys/xAOD/TopAnalysis/Root/EventSaverFlatNtuple.cxx

This merge request affects 36 packages. Since this is a long list, it will not be printed.

Adding @jchapman ,@bmondal ,@spalazzo ,@dshope ,@martindl ,@krumnack ,@jveatch ,@xiaozhon ,@fsforza ,@mleblanc ,@jojungge ,@markowen ,@oducu ,@ahaas ,@tdado ,@maklein ,@yoyamagu ,@adbailey ,@goetz ,@meehan ,@amorley ,@rnewhous ,@cdelitzs ,@nkoehler ,@tkharlam ,@sroe ,@nakahama ,@szambito ,@omajersk ,@ssnyder ,@akraszna ,@sschramm ,@tadej ,@gartoni ,@rbianchi ,@mvanadia as watchers

CI Result FAILURE (hash 6fe209d1)

	Athena	AthSimulation	AthGeneration	AnalysisBase	AthAnalysis
externals
cmake
make
required tests
optional tests

Full details available on this CI monitor view
Athena: number of compilation errors 1, warnings 0
AthSimulation: number of compilation errors 0, warnings 0
AthGeneration: number of compilation errors 0, warnings 0
AnalysisBase: number of compilation errors 0, warnings 0
AthAnalysis: number of compilation errors 0, warnings 0
For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 25962]

• The upside of having all these packages together - easier to review the generic aspects of the changes.
• Downside - how many people actually need to sign off on the package changes?
• Also, merge conflicts have been picked up.

I think the make failure is not due to anything in this MR. However, given the number of files touched I think we need to get back to a green board.

Please fix the merge conflict, and then let's re-run a build!