Fix include order by pinkenburg · Pull Request #4298 · sPHENIX-Collaboration/coresoftware

pinkenburg · 2026-06-11T23:41:11Z

Types of changes

[X ] Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work for users)
Requiring change in macros repository (Please provide links to the macros pull request in the last section)
I am a member of GitHub organization of sPHENIX Collaboration, EIC, or ECCE (contact Chris Pinkenburg to join)

What kind of change does this PR introduce? (Bug fix, feature, ...)

We had a bunch of sources where the include order was not correct, include "" before include <>: https://wiki.sphenix.bnl.gov/index.php?title=Codingconventions#Include_Files which can lead to very hard to debug problems when using private builds. This touched a few files which fail clang-tidy, I don't expect this PR to succeed on the first try

TODOs (if applicable)

Links to other PRs in macros and calibration repositories (if applicable)

Summary

This pull request addresses include-file ordering issues and modernizes member initialization across 37 source and header files to comply with the repository's coding convention (placing project-local includes before system/third-party includes) and resolve clang-tidy failures that occurred when using private builds.

Key Changes

Include Order Refactoring:

Reorganized include directives in headers throughout the codebase to place local/project-specific includes (quoted "") before system/external includes (angled <>), affecting files across calibration, generator, offline packages, and simulation modules
Adjusted include ordering for files in trackbase, trackbase_historic, trackreco, g4simulation, and phgenfit modules

Member Initialization Modernization:

Replaced empty inline destructors ({}) with defaulted destructors (= default) in several classes (GlobalVertexMap, TrainingHitsContainer, others)
Updated member default initializers from assignment style (= nullptr) to brace initialization ({nullptr}) for consistency
Converted pointer initialization syntax in private member sections across dEdxFitter, flowAfterburner, MvtxCombinedRawDataDecoder, and related classes

Standard Library Header Updates:

Changed <climits> to <limits> and replaced FLT_MAX usage with std::numeric_limits<float>::max() in KFParticle_Tools.cc and related headers
Added missing ROOT histogram header (TH1.h) to StreamingBcoLumiReco.cc

Code Cleanup:

Removed unused Init() and End() method overrides from MvtxCombinedRawDataDecoder that unconditionally returned EVENT_OK
Updated method signatures to use explicit parameter names instead of commented-out dummy parameters

Include Path Modernization:

Changed some includes from quoted to angled format where appropriate (e.g., RtypesCore.h, PHG4DisplayAction.h)
Added forward declarations instead of full includes where beneficial (e.g., TH1 in StreamingBcoLumiReco.h)

Potential Risk Areas

Include dependency changes: Reordering includes and switching between quoted/angled formats could expose hidden dependencies; verification that all includes resolve correctly is important
Removed methods: Deletion of Init() and End() overrides assumes default behavior is acceptable; confirm no derived classes depend on these method signatures
Standard library header changes: Switching from <climits> to <limits> and using std::numeric_limits should be functionally equivalent but may affect compilation on non-standard platforms
Member initialization changes: Brace-initialization and = default are functionally equivalent to previous forms but may interact differently with move semantics or copy operations in edge cases

Notes

The AI-generated change summaries may contain errors in interpretation. Manual verification of include dependency chains and method removal impacts is recommended before merging.

Possible Future Improvements

Establish automated linting to enforce consistent include ordering on submission
Complete modernization of remaining empty destructors and constructors across the codebase
Audit for additional opportunities to use standard library utilities (e.g., std::numeric_limits) in place of platform-specific macros

coderabbitai · 2026-06-11T23:41:28Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4eb80269-4ea8-4314-b4f1-91011614be35

📥 Commits

Reviewing files that changed from the base of the PR and between 04c646e and 0d58a9a.

📒 Files selected for processing (1)

simulation/g4simulation/g4gdml/PHG4GDMLAuxStructType.hh

✅ Files skipped from review due to trivial changes (1)

simulation/g4simulation/g4gdml/PHG4GDMLAuxStructType.hh

📝 Walkthrough

Walkthrough

The PR standardizes C++ include organization, numeric limit usage, and class initialization across 44 files spanning reconstruction, calibration, generator, and simulation code. Changes replace legacy floating-point constants with std::numeric_limits, unify member initialization to brace-syntax, modernize trivial special members with = default, remove unused lifecycle overrides, and reorder includes for dependency clarity.

Changes

Repository-wide nonfunctional cleanup

Layer / File(s)	Summary
Numeric limit standardization `calibrations/tpc/dEdx/dEdxFitter.h`, `offline/packages/trackbase_historic/SvtxTrackSeed_v2.h`, `offline/packages/trackbase/TrkrClusterv6.h`, `offline/packages/KFParticle_sPHENIX/KFParticle_Tools.cc`	`FLT_MAX`, `SHRT_MAX`, and `<climits>` are replaced with `std::numeric_limits<float>::max()` and `<limits>` throughout initialization defaults and verbose selection-check bounds.
Class/member and lifecycle modernization `generators/flowAfterburner/flowAfterburner.h`, `offline/packages/globalvertex/GlobalVertexMap.h`, `offline/packages/mvtx/MvtxCombinedRawDataDecoder.h`, `offline/packages/mvtx/MvtxCombinedRawDataDecoder.cc`, `offline/packages/tpc/TrainingHitsContainer.h`, `offline/packages/trackbase/ActsGeometry.cc`, `offline/packages/trackbase_historic/TrackInfoContainer_v3.h`, `offline/database/cdbobjects/CDBTTree.cc`	Member default initializers switch to brace-init syntax; destructors and constructors use `= default` instead of empty bodies; trivial `Init()` and `End()` lifecycle overrides are removed from `MvtxCombinedRawDataDecoder`.
Type and include contract adjustments `offline/packages/bcolumicount/StreamingBcoLumiReco.h`, `offline/packages/bcolumicount/StreamingBcoLumiReco.cc`, `offline/packages/PHGenFitPkg/PHGenFit/Fitter.h`, `offline/packages/trackreco/PHActsTrackProjection.h`	`TH1I` histogram pointers narrow to `TH1` with forward declarations; include paths and priorities are adjusted for correct type availability; duplicate includes are consolidated.
Offline include-order normalization `offline/QA/KFParticle/QAKFParticle.h`, `offline/packages/TruthNeutralMesonFinder/TruthNeutralMesonv1.h`, `offline/packages/globalvertex/GlobalVertexReco.h`, `offline/packages/trackbase/MvtxEventInfov1.h`, `offline/packages/trackbase/MvtxEventInfov2.h`, `offline/packages/trackbase/MvtxEventInfov3.h`, `offline/packages/trackbase/ResidualOutlierFinder.h`, `offline/packages/trackbase/SpacePoint.h`, `offline/packages/trackbase/TrkrClusterv2.h`, `offline/packages/trackbase/TrkrClusterv3.h`, `offline/packages/trackbase/TrkrClusterv4.h`, `offline/packages/trackbase/TrkrClusterv5.h`, `offline/packages/trackbase_historic/TrackAnalysisUtils.cc`, `offline/packages/trackreco/ALICEKF.h`	Includes are reordered to place project headers before standard library, forward declarations appear before heavy includes, and blank-line formatting is standardized.
Generator and simulation include-chain cleanup `generators/sHijing/xml_test.cc`, `simulation/g4simulation/g4eval/compressor_generator.h`, `simulation/g4simulation/g4eval/g4evalfn.cc`, `simulation/g4simulation/g4gdml/PHG4GDMLWrite.hh`, `simulation/g4simulation/g4gdml/PHG4GDMLWriteDefine.hh`, `simulation/g4simulation/g4gdml/PHG4GDMLWriteMaterials.hh`, `simulation/g4simulation/g4gdml/PHG4GDMLWriteSolids.hh`, `simulation/g4simulation/g4gdml/PHG4GDMLWriteStructure.hh`, `simulation/g4simulation/g4gdml/PHG4GDMLAuxStructType.hh`, `simulation/g4simulation/g4mvtx/PHG4MvtxDisplayAction.cc`, `simulation/g4simulation/g4tpc/PHG4TpcPadPlane.h`, `offline/packages/uspin/SpinDBNode.cc`	Local and project headers are moved ahead of external/Geant4/ROOT headers; Fortran macro setup is relocated earlier; include path style changes from quoted to angled where appropriate; missing Geant4 types are explicitly included.

Possibly Related PRs

sPHENIX-Collaboration/coresoftware#4269: This PR's adjustments to StreamingBcoLumiReco.h/.cc (histogram type narrowing from TH1I to TH1, include/forward-declaration updates) directly support the new streaming lumi/BCO tagging module introduced in PR #4269.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

offline/packages/trackbase/ResidualOutlierFinder.h (1)

27-32: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Raw ROOT object allocations lack RAII semantics and expose memory lifetime risks.

Lines 27, 29, and 31–32 use bare new to initialize hChi2, hDistance, and tree within the struct definition, with no corresponding delete in a destructor or RAII wrapper (e.g., std::unique_ptr). Because ResidualOutlierFinder defines no explicit destructor or deleted copy/move operations, default memberwise copying/moving will occur, creating double-delete and use-after-free hazards.

Define an explicit destructor to clean up these pointers, or wrap them in appropriate RAII containers (std::unique_ptr<TH2F>, std::unique_ptr<TNtuple>, or a smart ROOT manager class if available).

Source: Coding guidelines