Back to original try. No change in previous classes. And new v6 class.#4291
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds TrkrClusterv6 (a TrkrCluster subclass), extends TrkrCluster with many new virtual getters and updated float sentinels, implements TrkrClusterv6 non-inline methods (identify, isValid, CopyFrom), and integrates the class into the build and ROOT LinkDef. ChangesTrkrClusterv6 Implementation
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 74a80369-6d69-410b-9c5b-60d3a6602b7f
📒 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
Comment on lines
+118
to
+120
| setRSize(source.getRSize()); | ||
| setPhiSize(source.getPhiSize()); | ||
| setZSize(source.getZSize()); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify sentinel-producing base getters and narrowing assignments in CopyFrom
rg -n -C2 'virtual (float getRSize|float getPhiSize|float getZSize|int getSLMix|int getSRMix|int getTLMix|int getTRMix)' offline/packages/trackbase/TrkrCluster.h
rg -n -C2 'set(RSize|PhiSize|ZSize|SLMix|SRMix|TLMix|TRMix)\(' offline/packages/trackbase/TrkrClusterv6.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1706
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect TrkrClusterv6.cc around the reported CopyFrom assignments (and any nearby mix/size members)
sed -n '90,170p' offline/packages/trackbase/TrkrClusterv6.cc
# Inspect TrkrCluster.h around the sentinel defaults for the base getters
sed -n '60,140p' offline/packages/trackbase/TrkrCluster.h
# Confirm the exact setter signatures/types used by TrkrClusterv6 (so we know what conversions happen)
rg -n "void set(RSize|PhiSize|ZSize|SLMix|SRMix|TLMix|TRMix)\(" offline/packages/trackbase/TrkrClusterv6.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 5306
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Line numbers for CopyFrom assignments in TrkrClusterv6.cc
rg -n "set(RSize|PhiSize|ZSize|SLMix|SRMix|TLMix|TRMix)\(source\.get" offline/packages/trackbase/TrkrClusterv6.cc
# Setter signatures/definitions in TrkrClusterv6.cc (parameter types)
rg -n "void set(RSize|PhiSize|ZSize|SLMix|SRMix|TLMix|TRMix)\s*\(" offline/packages/trackbase/TrkrClusterv6.cc
# If setters are inline or inherited, also search for "setRSize" declarations
rg -n "set(RSize|PhiSize|ZSize|SLMix|SRMix|TLMix|TRMix)\s*[,)]" offline/packages/trackbase/TrkrClusterv6.cc | head -n 50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 327
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where setters are declared/overridden in TrkrClusterv6.cc (or any directly-included header within same file scope)
rg -n "setRSize|setPhiSize|setZSize|setSLMix|setSRMix|setTLMix|setTRMix" offline/packages/trackbase/TrkrClusterv6.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 327
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate setter signatures in TrkrCluster.h (or others) to confirm conversion targets/types
rg -n "set(RSize|PhiSize|ZSize|SLMix|SRMix|TLMix|TRMix)\s*\(" offline/packages/trackbase/TrkrCluster.h
# If not present there, search within trackbase for declarations
rg -n "set(RSize|PhiSize|ZSize|SLMix|SRMix|TLMix|TRMix)\s*\(" offline/packages/trackbase | head -n 50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Broader search for setter declarations/definitions (type-agnostic)
rg -n "set(RSize|PhiSize|ZSize|SLMix|SRMix|TLMix|TRMix)\s*\(" offline/packages/trackbase | head -n 200
# Also search for plain names without "(" in case of different syntax (e.g., member functions declared in classes)
rg -n "set(RSize|PhiSize|ZSize|SLMix|SRMix|TLMix|TRMix)" offline/packages/trackbase | head -n 200Repository: sPHENIX-Collaboration/coresoftware
Length of output: 4010
Guard sentinel-to-narrow assignments in TrkrClusterv6::CopyFrom to prevent corrupted cluster state.
TrkrCluster base getters return NAN for getRSize/getPhiSize/getZSize and std::numeric_limits<int>::max() for getSLMix/getSRMix/getTLMix/getTRMix. TrkrClusterv6::CopyFrom then writes these directly into narrow types (setRSize(unsigned char), setPhiSize(char), setZSize(char), and set*Mix(char)) at lines 118-120 and 131-134, so NAN→integer conversion is undefined and INT_MAX→char produces invalid/corrupted values for non-v6 sources.
Add sentinel/range checks (or only copy these fields when the source is known-valid for v6) before calling the narrow setters.
Build & test reportReport for commit 6f71eb405aa24369a8d075a22214b0d7741be0ee:
Automatically generated by sPHENIX Jenkins continuous integration |
…should take care of this
Build & test reportReport for commit badf5c7fab8c9abd5f02f6ca80bef2620ab9759f:
Automatically generated by sPHENIX Jenkins continuous integration |
osbornjd
approved these changes
Jun 9, 2026
osbornjd
left a comment
osbornjd
left a comment
Contributor
There was a problem hiding this comment.
I think this is good to go now unless @pinkenburg has any other requests
Build & test reportReport for commit 95f0073ffcaf5a2772ccd6ad67ec11fbe2787d19:
Automatically generated by sPHENIX Jenkins continuous integration |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
that I uses.
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)
Motivation / context
Key changes
Potential risk areas
Possible future improvements
Caveat