Adding new variables#4285
Conversation
📝 WalkthroughWalkthroughAdds TrkrClusterv6: extends TrkrCluster with new virtual getters, provides a full v6 implementation (constructor, identify, isValid, CopyFrom), and integrates the new class into the package build and ROOT dictionary. ChangesTrkrClusterv6 Variant Implementation
(Additional edits in this PR: ActsGeometry neighbor-surface change and ClusterErrorPara refactor plus many formatting/include/LinkDef tweaks across trackbase; see hidden review stack artifact for ranged mapping.) Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)offline/packages/trackbase/ActsGeometry.ccIn file included from offline/packages/trackbase/ActsGeometry.cc:1: ... [truncated 2200 characters] ... ns.CTrans_funct.instruction_log.(fun) in file "src/clang/cTrans.ml", line 4782, characters 12-47 offline/packages/trackbase/AlignmentTransformation.ccIn file included from offline/packages/trackbase/AlignmentTransformation.cc:1: ... [truncated 2200 characters] ... ile "src/clang/cTrans.ml", line 4866, characters 22-60 offline/packages/trackbase/ClusterErrorPara.ccIn file included from offline/packages/trackbase/ClusterErrorPara.cc:1: ... [truncated 1163 characters] ... clude"
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e308f7c3-fd1f-4f4a-b0a6-50240df0b478
📒 Files selected for processing (5)
offline/packages/trackbase/Makefile.amoffline/packages/trackbase/TrkrCluster.hoffline/packages/trackbase/TrkrClusterv6.ccoffline/packages/trackbase/TrkrClusterv6.hoffline/packages/trackbase/TrkrClusterv6LinkDef.h
| virtual unsigned int getCenAdc() const { return UINT_MAX; } | ||
| virtual float getPadCen() const { return NAN; } | ||
| virtual float getTBinCen() const { return NAN; } | ||
| virtual float getPadMax() const { return NAN; } | ||
| virtual float getTBinMax() const { return NAN; } | ||
| virtual char getSLEdge() const { return std::numeric_limits<char>::max(); } | ||
| virtual char getSREdge() const { return std::numeric_limits<char>::max(); } | ||
| virtual char getTLEdge() const { return std::numeric_limits<char>::max(); } | ||
| virtual char getTREdge() const { return std::numeric_limits<char>::max(); } | ||
| virtual char getDLEdge() const { return std::numeric_limits<char>::max(); } | ||
| virtual char getDREdge() const { return std::numeric_limits<char>::max(); } | ||
| virtual char getHLEdge() const { return std::numeric_limits<char>::max(); } | ||
| virtual char getHREdge() const { return std::numeric_limits<char>::max(); } | ||
| virtual int getSLMix() const { return std::numeric_limits<int>::max(); } | ||
| virtual int getSRMix() const { return std::numeric_limits<int>::max(); } | ||
| virtual int getTLMix() const { return std::numeric_limits<int>::max(); } | ||
| virtual int getTRMix() const { return std::numeric_limits<int>::max(); } | ||
| virtual float getPhiBinLo() const { return NAN; } | ||
| virtual float getPhiBinHi() const { return NAN; } | ||
| virtual float getTBinLo() const { return NAN; } | ||
| virtual float getTBinHi() const { return NAN; } | ||
| virtual float getPadPhase() const { return NAN; } | ||
| virtual float getTBinPhase() const { return NAN; } | ||
| virtual float getRSize() const { return NAN; } |
There was a problem hiding this comment.
Document ABI/API compatibility impact of the new virtual interface.
Adding these virtual getters to TrkrCluster changes the base-class vtable contract. Please include compatibility notes and required downstream rebuild/update guidance for any consumer built against older TrkrCluster headers.
As per coding guidelines, if interfaces change, ask for compatibility notes and any needed downstream updates.
| setRSize(source.getRSize()); | ||
| setPhiSize(source.getPhiSize()); | ||
| setZSize(source.getZSize()); | ||
| setOverlap(source.getOverlap()); | ||
| setEdge(source.getEdge()); | ||
| setSLEdge(source.getSLEdge()); | ||
| setSREdge(source.getSREdge()); | ||
| setTLEdge(source.getTLEdge()); | ||
| setTREdge(source.getTREdge()); | ||
| setDLEdge(source.getDLEdge()); | ||
| setDREdge(source.getDREdge()); | ||
| setHLEdge(source.getHLEdge()); | ||
| setHREdge(source.getHREdge()); | ||
| setSLMix(source.getSLMix()); | ||
| setSRMix(source.getSRMix()); | ||
| setTLMix(source.getTLMix()); | ||
| setTRMix(source.getTRMix()); |
There was a problem hiding this comment.
Type-gate v6-only field copies to avoid invalid state propagation.
This block copies v6-only attributes from a generic TrkrCluster. For non-v6 sources, the base getters return sentinels (see TrkrCluster.h Line 95-Line 105), which then get narrowed into v6 storage (char-backed mix/size fields), corrupting copied cluster state. Please gate these assignments behind a dynamic_cast<const TrkrClusterv6*> (or explicit sentinel filtering) and reset v6-only fields when the cast fails.
As per coding guidelines, prioritize correctness, memory safety, error handling, and thread-safety.
| #ifndef TRACKBASE_TRKRCLUSTERV6_H | ||
| #define TRACKBASE_TRKRCLUSTERV6_H | ||
|
|
||
| #include <iostream> |
There was a problem hiding this comment.
Please follow our include ordering from
https://wiki.sphenix.bnl.gov/index.php?title=Codingconventions#Include_Files
it does matter here. I notice that this is a problem in other include files (e.g. v5), that should be fixed asap
There was a problem hiding this comment.
Isn't that's how it is done. TrkrCluster and TrkrDefs are local inside trackbase. iostream is in <>.
| // float getPhiError() const override | ||
| //{ std::cout << "Deprecated getPhiError function"<< std::endl; return NAN;} | ||
|
|
||
| protected: |
There was a problem hiding this comment.
Please make members private, not protected
| //{ std::cout << "Deprecated getPhiError function"<< std::endl; return NAN;} | ||
|
|
||
| protected: | ||
| float m_local[2]{}; //< 2D local position [cm] 2 * 32 64bit - cumul 1*64 |
There was a problem hiding this comment.
Since m_local is only size 2, you should use
float m_local[2] {std::numeric_limits::quiet_NaN(), std::numeric_limits::quiet_NaN()};
to initialize them to NAN
| float m_padphase; | ||
| float m_tbinphase; | ||
|
|
||
| ClassDefOverride(TrkrClusterv6, 2) |
There was a problem hiding this comment.
Please start with version 1, not 2. (just FYI, 0 means no streamer which we only use for our virtual base classes which do not have data members)
| int getTLMix() const override { return (int) m_tlmix; } | ||
| void setTLMix(char tlmix) { m_tlmix = tlmix; } | ||
|
|
||
| int getTRMix() const override { return (int) m_trmix; } |
There was a problem hiding this comment.
you do not need the cast here, char automatically promotes to int:
int getTLMix() const override { return m_tlmix; }
| float m_tbincen; | ||
| float m_padmax; | ||
| float m_tbinmax; | ||
| char m_rsize; // 8bit |
There was a problem hiding this comment.
Are you sure you want char, not unsigned char? If your value is > 127, it will go negative
Build & test reportReport for commit b2a8a4737fca38a31649879c8db38936a2a1ca2b:
Automatically generated by sPHENIX Jenkins continuous integration |
There was a problem hiding this comment.
Actionable comments posted: 1
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/TrkrClusterv6.h (1)
261-264:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSigned
charfor mix members risks data corruption for values > 127.The getter methods (
getSLMix(), etc., lines 198-208) returnint, and the base class usesstd::numeric_limits<int>::max()as a sentinel. However, the backing members use signedcharwith range -128 to 127. Values greater than 127 will be stored as negative numbers, corrupting the data. Useunsigned charinstead.🔧 Proposed fix
- char m_slmix; // 8bit - char m_srmix; // 8bit - char m_tlmix; // 8bit - char m_trmix; // 8bit + unsigned char m_slmix; // 8bit + unsigned char m_srmix; // 8bit + unsigned char m_tlmix; // 8bit + unsigned char m_trmix; // 8bitBased on learnings from pinkenburg's previous review: signed char values > 127 go negative, which is typically not the intended behavior for these fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: db6a7132-57c6-4730-93bc-8d505d032109
📒 Files selected for processing (1)
offline/packages/trackbase/TrkrClusterv6.h
Build & test reportReport for commit f25df97ad25582e4432531984623b6588bf53e25:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 95b6c443a589719984662ee1c2c66b4e5667b797:
Automatically generated by sPHENIX Jenkins continuous integration |
|
I don't know about any of these clang-tidy errors. Those changes were not made by me. So can you please add this or do I have to change these clang tidy fixes myself? |
|
The clang-tidy warning is just because constexpr implies inline, so it is redundant. You should be able to just remove the |
|
You can always just run the following command on your code and it will fix the clang-tidy warnings for you https://wiki.sphenix.bnl.gov/index.php?title=Tools#clang_tidy |
There was a problem hiding this comment.
Actionable comments posted: 4
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/ActsGeometry.cc (1)
202-218:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap the
-1neighbor with signed arithmetic.Line 208 mixes
unsigned int nsurfwithi = -1. Whennsurfis 0,nsurf + iunderflows before the modulo, so this fallback probes an unrelated surface instead of the previous one. Near the TPC φ boundary, that can return the wrong surface ornullptrfor valid coordinates.Suggested fix
- unsigned int new_nsurf = (nsurf + i) % surf_vec.size(); + const auto surf_count = static_cast<int>(surf_vec.size()); + const auto new_nsurf = static_cast<unsigned int>( + (static_cast<int>(nsurf) + i + surf_count) % surf_count);As per coding guidelines, C++ changes here should prioritize correctness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c1d2f577-bf77-4108-84f5-b8ded5992e29
📒 Files selected for processing (64)
offline/packages/trackbase/.ActsGsfTrackFittingAlgorithm.h.swooffline/packages/trackbase/ActsGeometry.ccoffline/packages/trackbase/ActsGeometry.hoffline/packages/trackbase/ActsGsfTrackFittingAlgorithm.hoffline/packages/trackbase/ActsTrackFittingAlgorithm.hoffline/packages/trackbase/ActsTrackingGeometry.hoffline/packages/trackbase/AlignmentTransformation.ccoffline/packages/trackbase/AlignmentTransformation.hoffline/packages/trackbase/Calibrator.hoffline/packages/trackbase/ClusterErrorPara.ccoffline/packages/trackbase/ClusterErrorPara.hoffline/packages/trackbase/CommonOptions.ccoffline/packages/trackbase/CommonOptions.hoffline/packages/trackbase/IBaseDetector.hoffline/packages/trackbase/InttDefs.ccoffline/packages/trackbase/LaserCluster.hoffline/packages/trackbase/LaserClusterContainer.hoffline/packages/trackbase/LaserClusterContainerLinkDef.hoffline/packages/trackbase/LaserClusterContainerv1.ccoffline/packages/trackbase/LaserClusterContainerv1.hoffline/packages/trackbase/LaserClusterContainerv1LinkDef.hoffline/packages/trackbase/LaserClusterLinkDef.hoffline/packages/trackbase/LaserClusterv1.ccoffline/packages/trackbase/LaserClusterv1.hoffline/packages/trackbase/LaserClusterv1LinkDef.hoffline/packages/trackbase/LaserClusterv2.ccoffline/packages/trackbase/LaserClusterv2.hoffline/packages/trackbase/LaserClusterv2LinkDef.hoffline/packages/trackbase/MagneticFieldOptions.ccoffline/packages/trackbase/MagneticFieldOptions.hoffline/packages/trackbase/Makefile.amoffline/packages/trackbase/MaterialWiper.hoffline/packages/trackbase/MvtxDefs.ccoffline/packages/trackbase/MvtxEventInfov2.hoffline/packages/trackbase/RawHitSetv1.hoffline/packages/trackbase/ResidualOutlierFinder.hoffline/packages/trackbase/SpacePoint.hoffline/packages/trackbase/TGeoDetectorWithOptions.ccoffline/packages/trackbase/TGeoDetectorWithOptions.hoffline/packages/trackbase/TpcDefs.ccoffline/packages/trackbase/TpcDefs.hoffline/packages/trackbase/TrackFitUtils.ccoffline/packages/trackbase/TrackFitUtils.hoffline/packages/trackbase/TrackFittingAlgorithmFunctionsKalman.ccoffline/packages/trackbase/TrackVertexCrossingAssoc_v1.ccoffline/packages/trackbase/TrkrClusterContainer.hoffline/packages/trackbase/TrkrClusterContainerv4.ccoffline/packages/trackbase/TrkrClusterHitAssoc.hoffline/packages/trackbase/TrkrClusterv6.hoffline/packages/trackbase/TrkrDefs.ccoffline/packages/trackbase/TrkrDefs.hoffline/packages/trackbase/TrkrHit.hoffline/packages/trackbase/TrkrHitSetContainerv2.ccoffline/packages/trackbase/TrkrHitSetContainerv2.hoffline/packages/trackbase/TrkrHitSetContainerv2LinkDef.hoffline/packages/trackbase/TrkrHitSetTpc.hoffline/packages/trackbase/TrkrHitSetTpcLinkDef.hoffline/packages/trackbase/TrkrHitSetTpcv1.hoffline/packages/trackbase/TrkrHitSetTpcv1LinkDef.hoffline/packages/trackbase/alignmentTransformationContainer.hoffline/packages/trackbase/autogen.shoffline/packages/trackbase/configure.acoffline/packages/trackbase/sPHENIXActsDetectorElement.ccoffline/packages/trackbase/sPHENIXActsDetectorElement.h
💤 Files with no reviewable changes (2)
- offline/packages/trackbase/alignmentTransformationContainer.h
- offline/packages/trackbase/TrackFittingAlgorithmFunctionsKalman.cc
✅ Files skipped from review due to trivial changes (53)
- offline/packages/trackbase/Calibrator.h
- offline/packages/trackbase/LaserClusterLinkDef.h
- offline/packages/trackbase/LaserClusterContainerLinkDef.h
- offline/packages/trackbase/TrkrHitSetTpcLinkDef.h
- offline/packages/trackbase/TrkrHitSetContainerv2LinkDef.h
- offline/packages/trackbase/TrkrHitSetTpcv1LinkDef.h
- offline/packages/trackbase/TrkrClusterContainer.h
- offline/packages/trackbase/TrkrClusterHitAssoc.h
- offline/packages/trackbase/ActsGeometry.h
- offline/packages/trackbase/InttDefs.cc
- offline/packages/trackbase/IBaseDetector.h
- offline/packages/trackbase/TrkrDefs.h
- offline/packages/trackbase/LaserClusterv2LinkDef.h
- offline/packages/trackbase/CommonOptions.h
- offline/packages/trackbase/MaterialWiper.h
- offline/packages/trackbase/SpacePoint.h
- offline/packages/trackbase/TrkrHitSetContainerv2.h
- offline/packages/trackbase/TpcDefs.cc
- offline/packages/trackbase/ActsTrackFittingAlgorithm.h
- offline/packages/trackbase/MvtxEventInfov2.h
- offline/packages/trackbase/LaserClusterContainerv1LinkDef.h
- offline/packages/trackbase/TrkrClusterContainerv4.cc
- offline/packages/trackbase/MagneticFieldOptions.h
- offline/packages/trackbase/sPHENIXActsDetectorElement.cc
- offline/packages/trackbase/MvtxDefs.cc
- offline/packages/trackbase/TrkrHitSetTpcv1.h
- offline/packages/trackbase/TGeoDetectorWithOptions.h
- offline/packages/trackbase/TpcDefs.h
- offline/packages/trackbase/TrkrHitSetContainerv2.cc
- offline/packages/trackbase/TrkrDefs.cc
- offline/packages/trackbase/AlignmentTransformation.h
- offline/packages/trackbase/LaserClusterContainerv1.h
- offline/packages/trackbase/TrkrHitSetTpc.h
- offline/packages/trackbase/TrackFitUtils.cc
- offline/packages/trackbase/RawHitSetv1.h
- offline/packages/trackbase/ActsTrackingGeometry.h
- offline/packages/trackbase/TrkrHit.h
- offline/packages/trackbase/sPHENIXActsDetectorElement.h
- offline/packages/trackbase/ActsGsfTrackFittingAlgorithm.h
- offline/packages/trackbase/TrackVertexCrossingAssoc_v1.cc
- offline/packages/trackbase/TGeoDetectorWithOptions.cc
- offline/packages/trackbase/AlignmentTransformation.cc
- offline/packages/trackbase/LaserClusterv1.cc
- offline/packages/trackbase/LaserClusterContainerv1.cc
- offline/packages/trackbase/ClusterErrorPara.h
- offline/packages/trackbase/LaserClusterContainer.h
- offline/packages/trackbase/TrackFitUtils.h
- offline/packages/trackbase/LaserCluster.h
- offline/packages/trackbase/LaserClusterv2.h
- offline/packages/trackbase/LaserClusterv1.h
- offline/packages/trackbase/CommonOptions.cc
- offline/packages/trackbase/LaserClusterv2.cc
- offline/packages/trackbase/MagneticFieldOptions.cc
🚧 Files skipped from review as they are similar to previous changes (2)
- offline/packages/trackbase/Makefile.am
- offline/packages/trackbase/TrkrClusterv6.h
| #!/ bin / sh | ||
| srcdir =`dirname $0` test - z "$srcdir" &&srcdir =. | ||
|
|
||
| (cd $srcdir; aclocal -I ${OFFLINE_MAIN}/share;\ | ||
| libtoolize --force; automake -a --add-missing; autoconf) | ||
|
|
||
| $srcdir/configure "$@" | ||
| (cd $srcdir; aclocal - I ${OFFLINE_MAIN} / share; | ||
| libtoolize-- force; automake - a-- add - missing; autoconf) | ||
|
|
||
| $srcdir / | ||
| configure "$@" |
There was a problem hiding this comment.
Critical: Shell syntax errors render the script non-functional.
The formatting changes introduce multiple syntax errors that will prevent this bootstrap script from executing:
- Line 1: The shebang
#!/ bin / shis invalid. The kernel expects a single interpreter path without spaces. - Line 2: Spaces around
=break variable assignment (srcdir =should besrcdir=). The command separator is also malformed. - Lines 4-5: Command options are broken by spaces (e.g.,
aclocal - Ishould beaclocal -I,libtoolize-- forceshould belibtoolize --force,automake - a-- add - missingshould beautomake -a --add-missing). - Lines 7-8: The path separator is split across lines (
$srcdir /andconfigureshould be$srcdir/configure).
These errors will cause immediate build failures when developers run autogen.sh. The script must use correct shell syntax to bootstrap the autotools build system.
🔧 Revert to correct shell syntax
-#!/ bin / sh
-srcdir =`dirname $0` test - z "$srcdir" &&srcdir =.
+#!/bin/sh
+srcdir=`dirname $0`; test -z "$srcdir" && srcdir=.
- (cd $srcdir; aclocal - I ${OFFLINE_MAIN} / share;
- libtoolize-- force; automake - a-- add - missing; autoconf)
+(cd $srcdir; aclocal -I ${OFFLINE_MAIN}/share; \
+ libtoolize --force; automake -a --add-missing; autoconf)
- $srcdir /
- configure "$@"
+$srcdir/configure "$@"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #!/ bin / sh | |
| srcdir =`dirname $0` test - z "$srcdir" &&srcdir =. | |
| (cd $srcdir; aclocal -I ${OFFLINE_MAIN}/share;\ | |
| libtoolize --force; automake -a --add-missing; autoconf) | |
| $srcdir/configure "$@" | |
| (cd $srcdir; aclocal - I ${OFFLINE_MAIN} / share; | |
| libtoolize-- force; automake - a-- add - missing; autoconf) | |
| $srcdir / | |
| configure "$@" | |
| #!/bin/sh | |
| srcdir=`dirname $0`; test -z "$srcdir" && srcdir=. | |
| (cd $srcdir; aclocal -I ${OFFLINE_MAIN}/share; \ | |
| libtoolize --force; automake -a --add-missing; autoconf) | |
| $srcdir/configure "$@" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[error] 1-1: On most OS, shebangs can only specify a single parameter.
(SC2096)
[error] 1-1: This shebang specifies a directory. Ensure the interpreter is a file.
(SC2246)
[error] 2-2: Remove spaces around = to assign (or use [ ] to compare, or quote '=' if literal).
(SC2283)
[warning] 2-2: Quote this to prevent word splitting.
(SC2046)
[style] 2-2: Use $(...) notation instead of legacy backticks ....
(SC2006)
[info] 2-2: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 2-2: srcdir is referenced but not assigned.
(SC2154)
[error] 2-2: Remove spaces around = to assign (or use [ ] to compare, or quote '=' if literal).
(SC2283)
[warning] 4-4: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[info] 4-4: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 4-4: Double quote to prevent globbing and word splitting.
(SC2086)
|
|
||
| AC_PROG_CXX(CC g++) | ||
| LT_INIT([disable-static]) | ||
| LT_INIT([disable - static]) |
There was a problem hiding this comment.
Critical: Invalid libtool option syntax.
The LT_INIT argument has been changed to [disable - static] with spaces, which is not a valid libtool option. The correct form is [disable-static] as a single hyphenated string. This will likely cause libtool initialization to fail or ignore the option, resulting in unwanted static library builds.
🔧 Restore correct option syntax
-LT_INIT([disable - static])
+LT_INIT([disable-static])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LT_INIT([disable - static]) | |
| LT_INIT([disable-static]) |
| *) AC_MSG_ERROR(bad value ${ | ||
| enableval} for --enable-online) ;; |
There was a problem hiding this comment.
Critical: Broken parameter expansion in error message.
The shell parameter expansion ${enableval} has been split across lines (${ on line 23 and enableval} on line 24). This breaks the shell variable substitution syntax and will prevent the error message from displaying the actual invalid value when --enable-online is given a bad argument.
🔧 Restore correct parameter expansion
- *) AC_MSG_ERROR(bad value ${
- enableval} for --enable-online) ;;
+ *) AC_MSG_ERROR(bad value ${enableval} for --enable-online) ;;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| *) AC_MSG_ERROR(bad value ${ | |
| enableval} for --enable-online) ;; | |
| *) AC_MSG_ERROR(bad value ${enableval} for --enable-online) ;; |
| struct ResidualOutlierFinder | ||
| { | ||
| ActsGeometry* m_tGeometry = nullptr; | ||
| ActsGeometry *m_tGeometry = nullptr; |
There was a problem hiding this comment.
Don’t terminate the whole job from this predicate on missing geometry.
m_tGeometry still defaults to nullptr, so any default-constructed or partially configured ResidualOutlierFinder now aborts the entire process via exit(1). That turns a local configuration error into an unrecoverable job failure and bypasses normal framework cleanup/reporting. Please surface this as a caller-visible failure instead of terminating from inside operator().
Safer fallback
- if (!m_tGeometry)
- {
- std::cout << PHWHERE << "no geometry set in residual outlier finder" << std::endl;
- exit(1);
- }
+ if (!m_tGeometry)
+ {
+ std::cout << PHWHERE << "no geometry set in residual outlier finder" << std::endl;
+ return false;
+ }As per coding guidelines, header changes should make ownership/lifetime requirements explicit and avoid unclear lifetime assumptions.
Also applies to: 78-81
|
I don't understand these ActsGsf errors. |
3 participants
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Pull Request Summary: Adding TrkrClusterv6 and new TrkrCluster accessors
Motivation / context
Key changes
Potential risk areas
Possible future improvements
Note about AI assistance