Use short int for vertex beam crossing#4122
Use short int for vertex beam crossing#4122pinkenburg merged 5 commits intosPHENIX-Collaboration:masterfrom
Conversation
📝 WalkthroughWalkthroughThe PR updates beam crossing method signatures from Changes
✨ Finishing touches
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}
| short int get_beam_crossing() const override { return _bco; } | ||
| void set_beam_crossing(short int bco) override { _bco = bco; } | ||
|
|
||
| private: | ||
| unsigned int _id{std::numeric_limits<unsigned int>::max()}; //< unique identifier within container | ||
| unsigned int _bco{std::numeric_limits<unsigned int>::max()}; //< global bco | ||
| short int _bco{std::numeric_limits<short int>::max()}; //< global bco |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "MbdVertexv2.h" -type fRepository: sPHENIX-Collaboration/coresoftware
Length of output: 123
🏁 Script executed:
cat -n offline/packages/globalvertex/MbdVertexv2.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 2522
🏁 Script executed:
cd offline/packages/globalvertex && git log --oneline MbdVertexv2.h | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 126
🏁 Script executed:
cd offline/packages/globalvertex && git diff HEAD~5 MbdVertexv2.h | head -100Repository: sPHENIX-Collaboration/coresoftware
Length of output: 264
🏁 Script executed:
cd offline/packages/globalvertex && git show 2963c3678:MbdVertexv2.h | grep -A 10 "_bco"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 275
🏁 Script executed:
cd offline/packages/globalvertex && git show 2963c3678 | head -100Repository: sPHENIX-Collaboration/coresoftware
Length of output: 3065
🏁 Script executed:
cd offline/packages/globalvertex && git log --all --oneline | grep -i "bco\|beam"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 20440
🏁 Script executed:
rg "ClassDefOverride.*MbdVertexv2" offline/packages/globalvertex/Repository: sPHENIX-Collaboration/coresoftware
Length of output: 157
🏁 Script executed:
git show 2963c3678:offline/packages/globalvertex/MbdVertexv2.h | tail -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 980
🏁 Script executed:
git show 2963c3678~1:offline/packages/globalvertex/MbdVertexv2.h | tail -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 119
🏁 Script executed:
rg "ClassDefOverride.*[vV]ertex" offline/packages/globalvertex/ -A 1 -B 5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 12583
🏁 Script executed:
cat -n offline/packages/globalvertex/MbdVertexv1.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 2049
🏁 Script executed:
cat -n offline/packages/globalvertex/MbdVertex.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 2261
🏁 Script executed:
rg "_bco" offline/packages/globalvertex/MbdVertex*.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 411
🏁 Script executed:
git log --all --oneline -- offline/packages/globalvertex/MbdVertexv2.h | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 466
🏁 Script executed:
git log --all -p -- offline/packages/globalvertex/MbdVertexv2.h | grep -A 30 "_bco" | head -60Repository: sPHENIX-Collaboration/coresoftware
Length of output: 2955
🏁 Script executed:
git show 2963c3678^:offline/packages/globalvertex/MbdVertexv2.h 2>/dev/null | tail -15Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
web_search
ROOT ClassDefOverride schema versioning requirements when member types changeRepository: sPHENIX-Collaboration/coresoftware
Length of output: 171
🌐 Web query:
ROOT ClassDefOverride schema versioning member type changes
💡 Result:
Short answer: when you change a member’s type you must increase the class version (ClassDef / ClassDefOverride) and either rely on ROOT’s automatic type-conversion rules (only a limited, well‑specified set of compatible conversions) or provide an explicit I/O customization rule (pragma read / custom streamer) that converts the on-file member into the new in‑memory type. Automatic evolution will not handle arbitrary type changes (e.g. vector<T*> → unrelated user type); for those you must supply a conversion rule or whole-object rule. See ROOT schema‑evolution and I/O customization docs for the allowed automatic conversions and pragma read syntax. [1][2]
Concrete pointers / actions
- Bump the ClassDef/ClassDefOverride version whenever you change layout. ROOT writes that version in the file and uses it for evolution. [2]
- Check whether the old on‑disk type → new in‑memory type is in ROOT’s automatic conversion tables (integral width changes, enum↔integer with bounds checks, some pointer/collection rules). If yes, no further work is needed. [1]
- If automatic rules do not apply, add a pragma read rule (in the header/LinkDef) targeting the old on‑disk version(s) and supply source=..., target=... and code={...} to perform the conversion (or use a whole-object rule). Example form shown in the docs. Ensure at runtime at most one rule applies per target member. [1][2]
- You can also use custom streamers or provide conversion constructors/operators, but explicit pragma read rules are the supported way to map arbitrary old-member types to new ones. [1]
References
- ROOT Schema Evolution (rules and automatic conversion tables). [1]
- ROOT I/O / ClassDef and I/O customization (ClassDefOverride and
#pragmaread examples). [2]
Bump ClassDefOverride version due to ROOT I/O schema change.
The change from unsigned int to short int for _bco (commit 2963c36) alters the serialized format and memory layout. ROOT requires a class version increment when member types change. Update ClassDefOverride(MbdVertexv2, 1) to ClassDefOverride(MbdVertexv2, 2), or provide a custom streamer/pragma read rule for backward compatibility with existing ROOT files. Unsigned-to-short conversion is not handled automatically by ROOT's schema evolution.
| short int get_beam_crossing() const override { return _beamcrossing; } | ||
| void set_beam_crossing(short int cross) override { _beamcrossing = cross; } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "SvtxVertex_v2.h" | head -5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 212
🌐 Web query:
ROOT I/O ClassDefOverride version bump schema change member variable type
💡 Result:
Short answer
- Yes — if you change the type of an immediate data member you must bump the ClassDef/ClassDefOverride version. Root’s schema-evolution relies on the class “version” to detect layout changes (member type, add/remove/rename immediate members). [1][2]
Key details and options
- ClassDefOverride behaves like ClassDef (but marks injected methods as override). The same versioning rules apply. Increase the integer in ClassDefOverride(ClassName, N) when the class data-layout changes. [1]
- You do NOT need to bump a class’s version when only a member of a base class changes (only immediate data-member changes matter). [3]
- If you change a member’s type but want to read old files you can:
- rely on ROOT’s automatic schema-evolution when the change is compatible; or
- provide custom I/O conversion using linkdef read/pragma rules or a custom Streamer to convert old-field values into the new type at read time. Use these when automatic conversion fails. Forum posts describe examples and pitfalls. [4][5]
When to write a custom Streamer / read rule
- If ROOT prints warnings like “Cannot convert … from type: X to type: Y, skip element”, automatic evolution failed and you should add a manual conversion (pragma read / custom Streamer) to map old-data to the new member/type. [4]
Sources
- ROOT I/O manual (ClassDef / versioning). [1]
- ROOT Users Guide / ClassDef description. [2]
- ROOT Forum discussion: when base changes vs immediate-member changes. [3]
- ROOT Forum thread on type-change conversion and manual schema-evolution (example and warning message). [4][5]
🌐 Web query:
SvtxVertex_v2 ROOT serialization unsigned int short int beam crossing
💡 Result:
Do you mean (A) which SvtxVertex_v2 member stores the beam/bunch-crossing and its C++ type, or (B) how ROOT serializes unsigned int vs unsigned short in SvtxVertex_v2? I can look up the SvtxVertex_v2 header and ROOT streamer info — which do you want me to fetch?
🌐 Web query:
ROOT schema evolution unsigned int to short int type change compatibility
💡 Result:
Short answer: ROOT will only convert between integer types when the conversion is safe or when you supply explicit schema-evolution code — narrowing from 32-bit unsigned (UInt_t) to 16-bit signed (Short_t) is not guaranteed automatically because it can overflow/lose information. See ROOT's type-conversion rules (bounds checks are enforced for narrowing) and the advice to use manual I/O customization when automatic rules aren't sufficient. [1][2]
Practical guidance
- Expect a bounds check / possible rejection or truncation when reading an on-file unsigned 32-bit into an in-memory signed 16-bit field — ROOT's automatic rules allow integer→integer conversions but require bounds checking for potentially unsafe conversions. If any on-disk value is outside the target range, the read will fail or the behavior will not be what you want. [1]
- Best practice: keep portable, explicit types (Int_t/UInt_t, Short_t/UShort_t) and avoid silently narrowing. If you must change the field from unsigned int → short:
- Bump the ClassDef version.
- Provide an I/O customization/schema-evolution rule (pragma read or TStreamer/streamerInfo-based code) that explicitly converts the old unsigned 32-bit value to the new short, performing range checks and defining the behavior for out-of-range values (clip, set sentinel, error, etc.). This is the recommended/robust approach. [1][2]
- Note: schema-evolution with TTrees has some historical caveats (manual rules may behave differently for objects stored in TTrees vs. directly in TFiles); test reading old files after adding your rule. [2]
References
- ROOT Schema Evolution (automatic and manual rules, type-conversion table). [1]
- ROOT forum discussion & examples about schema evolution and TTree specifics (manual rules / pragma read usage). [2]
ROOT I/O schema change: ClassDefOverride version bump required.
Narrowing _beamcrossing from unsigned int to short int alters the serialized schema and creates a potentially unsafe type conversion (32-bit unsigned → 16-bit signed). Bump ClassDefOverride(SvtxVertex_v2, 2) to version 3, and provide a custom streamer or ROOT schema-evolution rule to safely convert existing on-disk values (which may exceed the range of short int).
| // beam crossing methods | ||
| virtual unsigned int get_beam_crossing() const { return std::numeric_limits<unsigned int>::max(); } | ||
| virtual void set_beam_crossing(unsigned int) {} | ||
| virtual short int get_beam_crossing() const { return std::numeric_limits<short int>::max(); } | ||
| virtual void set_beam_crossing(short int) {} |
There was a problem hiding this comment.
Document/enforce the new range and note ABI impact.
This API narrows from unsigned int to short int, which is a breaking change and can silently truncate if any producer emits values > 32767. Please add explicit range guarantees (or checks in concrete setters) and include compatibility notes for downstream users.
|
This PR needs work - the signed versus unsigned int needs sorting out (remember we want to be able to read old files) and this is a schema evolution which needs to be handled with at least a version change. Sometimes even this breaks the readback of old files which needs to be verified |
Build & test reportReport for commit 2963c36785cc61b40722e0139177340f09977c7a:
Automatically generated by sPHENIX Jenkins continuous integration |
|
The use of short int in PHSimpleVertexFinder is deliberate - the crossings can be negative. The values default to SHORT_MAX if they are undetermined. Converting to unsigned int will produce errors. |
|
@adfrawley this is what this PR attempts to address, currently the type that is returned is If we need to introduce a new version for it then so be it - but technically the current code is wrong |
|
@osbornjd @pinkenburg Could you please take a look at the modified files and let me know if everything looks ok, or if any additional changes are needed? |
Build & test reportReport for commit 47036fb2082144f30e9d21df6f77fc170464f26d:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 8fb707e05cbc56c6c6f81f8b83a5b6ecbf6c15d0:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit 3c966d3934fc23cb1be61bef7171d51167de2140:
Automatically generated by sPHENIX Jenkins continuous integration |
osbornjd
left a comment
osbornjd
left a comment
There was a problem hiding this comment.
Can you also update where the vertices are created to use v3 instead of v2. For example in PHSimpleVertexFinder where the new vertex is created, just bump the version from v2 to v3. Same for MbdVertexv2and GlobalVertexv2
|
Okay, I think this is good to go then unless Chris has any other suggestions |
|
All the clang-tidy seem to come from the new version. The test tracking reconstruction is due to database connection? |
|
yes - no clang-tidy problem (the clang-tidy errors are from the include file change which isn't propagated to other packages) The db is a load problem - also not caused by this PR |
| short int get_beam_crossing() const override | ||
| { | ||
| return rollover_from_unsignedint(_bco); | ||
| } | ||
|
|
||
| void set_beam_crossing(short int bco) override | ||
| { | ||
| if (bco == short_int_max) | ||
| { | ||
| _bco = std::numeric_limits<unsigned int>::max(); | ||
| return; | ||
| } | ||
|
|
||
| const short int bco_ro = rollover_short(bco); | ||
| _bco = static_cast<unsigned int>(bco_ro); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and inspect the file
find . -name "GlobalVertexv2.h" -type fRepository: sPHENIX-Collaboration/coresoftware
Length of output: 126
🏁 Script executed:
# Get file size and read the relevant sections
if [ -f "offline/packages/globalvertex/GlobalVertexv2.h" ]; then
wc -l offline/packages/globalvertex/GlobalVertexv2.h
fiRepository: sPHENIX-Collaboration/coresoftware
Length of output: 128
🏁 Script executed:
# Read the complete file to understand the context
cat -n offline/packages/globalvertex/GlobalVertexv2.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 4492
🏁 Script executed:
# Search for any tests or documentation about beam crossing behavior
find . -type f \( -name "*.cpp" -o -name "*.h" -o -name "*.txt" -o -name "*.md" \) | \
xargs grep -l "get_beam_crossing\|set_beam_crossing\|rollover_short\|rollover_from_unsignedint" 2>/dev/null | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 495
🏁 Script executed:
# Search for tests related to beam crossing
find . -type f \( -name "*test*.cpp" -o -name "*Test*.cpp" \) | \
xargs grep -l "beam_crossing\|GlobalVertex" 2>/dev/null | head -10Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Search for any documentation or comments about legacy data format
rg -i "legacy|beam.*cross|bco" --type h --type cpp -B 2 -A 2 | head -100Repository: sPHENIX-Collaboration/coresoftware
Length of output: 7067
🏁 Script executed:
# Check GlobalVertex.h for any interface documentation
cat -n offline/packages/globalvertex/GlobalVertex.h | head -150Repository: sPHENIX-Collaboration/coresoftware
Length of output: 5606
🏁 Script executed:
# Look at other versions to understand the evolution
cat -n offline/packages/globalvertex/GlobalVertexv3.h | grep -A 20 "beam_crossing\|rollover"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1080
🏁 Script executed:
# Let's trace through a concrete example to verify the rollover logic
# Example: set_beam_crossing(-100)
python3 << 'EOF'
# Simulate the current logic
short_int_max = 32767
def rollover_short(bco):
if bco == short_int_max:
return short_int_max
if bco >= 0:
return bco
# For negatives
return short_int_max + bco
def rollover_from_unsignedint(bco):
if bco == (2**32 - 1): # UINT_MAX
return short_int_max
if bco <= short_int_max:
return bco if bco < 32768 else (bco - 65536) # interpret as signed short
# Simulate: cast to unsigned short, then to short int
bco_short = bco & 0xFFFF # Take lower 16 bits
# Interpret as signed
if bco_short >= 32768:
bco_ro = bco_short - 65536 # negative
else:
bco_ro = bco_short # positive
if bco_ro >= 0:
return bco_ro
return rollover_short(bco_ro)
# Test with negative value
test_input = -100
print(f"Setting beam_crossing to {test_input}")
bco_ro = rollover_short(test_input)
print(f"After rollover_short: {bco_ro}")
stored = bco_ro & 0xFFFFFFFF # store as unsigned int
print(f"Stored as unsigned: {stored}")
retrieved = rollover_from_unsignedint(stored)
print(f"Retrieved: {retrieved}")
print(f"Round-trip successful: {test_input == retrieved}")
# Compare with simple cast approach
simple_stored = (-100 & 0xFFFFFFFF) & 0xFFFF # lower 16 bits
simple_retrieved = simple_stored if simple_stored < 32768 else (simple_stored - 65536)
print(f"\nWith simple cast approach:")
print(f"Stored: {simple_stored}")
print(f"Retrieved: {simple_retrieved}")
print(f"Round-trip successful: {test_input == simple_retrieved}")
EOFRepository: sPHENIX-Collaboration/coresoftware
Length of output: 293
🏁 Script executed:
# Check if there are any version 1 classes for reference
find . -name "*GlobalVertex*" -type f | xargs ls -la | grep -v ".o"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Search for any documentation or code comments about the purpose of rollover logic
rg -i "rollover|two.s.complement|legacy|negat" offline/packages/globalvertex/ -B 2 -A 2Repository: sPHENIX-Collaboration/coresoftware
Length of output: 8090
🏁 Script executed:
# Verify the suggested fix preserves two's complement semantics
python3 << 'EOF'
# Verify the proposed fix is correct
def proposed_fix_set(bco):
"""Proposed fix for set_beam_crossing"""
short_int_max = 32767
if bco == short_int_max:
return (1 << 32) - 1 # UINT_MAX
# Direct cast: short int -> unsigned short -> unsigned int
return (bco & 0xFFFF)
def proposed_fix_get(bco_unsigned):
"""Proposed fix for get_beam_crossing"""
short_int_max = 32767
if bco_unsigned == ((1 << 32) - 1): # UINT_MAX
return short_int_max
# Direct cast: unsigned int -> unsigned short -> short int
bco_short = bco_unsigned & 0xFFFF
# Convert to signed
if bco_short >= 32768:
return bco_short - 65536
return bco_short
# Test round-trip with various values
test_values = [-100, -1, 0, 1, 100, 32766, 32767]
print("Testing proposed fix:")
for val in test_values:
stored = proposed_fix_set(val)
retrieved = proposed_fix_get(stored)
status = "✓" if val == retrieved else "✗"
print(f"{status} {val:6d} → {stored:10d} → {retrieved:6d}")
# Also test legacy behavior (simulating old unsigned int cast)
print("\nTesting legacy compatibility (old code writing as unsigned int):")
for val in [-100, -1]:
# Old code would do: _bco = static_cast<unsigned int>(bco); // where bco is short int
# This is undefined behavior but typically truncates the lower 16 bits
legacy_stored = val & 0xFFFFFFFF # Direct cast
retrieved = proposed_fix_get(legacy_stored)
print(f"Legacy value {val} stored as {legacy_stored} → retrieved as {retrieved}")
EOFRepository: sPHENIX-Collaboration/coresoftware
Length of output: 499
🏁 Script executed:
# Check if there are any v1 versions that don't have this rollover logic
cat -n offline/packages/globalvertex/GlobalVertexv1.h 2>/dev/null | grep -A 10 "beam_crossing" || echo "No v1 found"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 89
🏁 Script executed:
# Search for any git history or comments about why rollover was introduced
git log --oneline --all -- offline/packages/globalvertex/GlobalVertexv2.h 2>/dev/null | head -10Repository: sPHENIX-Collaboration/coresoftware
Length of output: 522
🏁 Script executed:
# Check if there are any tests that would catch this data corruption
find . -type f \( -name "*test*.cpp" -o -name "*Test*.cpp" \) | \
xargs grep -l "beam_crossing\|GlobalVertexv2\|SvtxVertex_v2\|MbdVertexv2" 2>/dev/null | head -5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Let's verify the impact: are there multiple files with the same bug?
echo "=== GlobalVertexv2.h ===" && grep -n "rollover_short\|rollover_from_unsignedint" offline/packages/globalvertex/GlobalVertexv2.h | wc -l
echo "=== SvtxVertex_v2.h ===" && grep -n "rollover_short\|rollover_from_unsignedint" offline/packages/globalvertex/SvtxVertex_v2.h | wc -l
echo "=== MbdVertexv2.h ===" && grep -n "rollover_short\|rollover_from_unsignedint" offline/packages/globalvertex/MbdVertexv2.h | wc -lRepository: sPHENIX-Collaboration/coresoftware
Length of output: 154
Beam-crossing rollover loses sign information, corrupting negative values.
The rollover_short() logic at lines 84–88 computes short_int_max + bco for negative inputs, which maps −100 → 32667 instead of preserving the two's-complement pattern (−100 → 0xFFC6 = 65436 unsigned). Consequently, set_beam_crossing(−100) followed by get_beam_crossing() returns 32667, not −100. This breaks round-trip integrity and undermines legacy file reading, where old data with negative crossings will be silently corrupted.
Replace the arithmetic rollover with direct unsigned-short casting to preserve two's-complement semantics:
Suggested fix
void set_beam_crossing(short int bco) override
{
if (bco == short_int_max)
{
_bco = std::numeric_limits<unsigned int>::max();
return;
}
-
- const short int bco_ro = rollover_short(bco);
- _bco = static_cast<unsigned int>(bco_ro);
+ _bco = static_cast<unsigned int>(static_cast<unsigned short>(bco));
}
static short int rollover_short(short int bco)
- {
- if (bco == short_int_max) return short_int_max;
- if (bco >= 0) return bco;
- return static_cast<short int>(static_cast<int>(short_int_max) + static_cast<int>(bco));
- }
-
static short int rollover_from_unsignedint(unsigned int bco)
{
if (bco == std::numeric_limits<unsigned int>::max()) return short_int_max;
- if (bco <= static_cast<unsigned int>(short_int_max)) return static_cast<short int>(bco);
-
- const short int bco_ro = static_cast<short int>(static_cast<unsigned short>(bco));
- if (bco_ro >= 0) return bco_ro;
- return rollover_short(bco_ro);
+ return static_cast<short int>(static_cast<unsigned short>(bco));
}Same issue and fix apply to SvtxVertex_v2.h and MbdVertexv2.h.
| void GlobalVertexv3::insert_vtx(GlobalVertex::VTXTYPE type, const Vertex* vertex) | ||
| { | ||
| auto it = _vtxs.find(type); | ||
| if (it == _vtxs.end()) | ||
| { | ||
| VertexVector vector; | ||
| vector.push_back(vertex); | ||
| _vtxs.insert(std::make_pair(type, vector)); | ||
| return; | ||
| } | ||
|
|
||
| it->second.push_back(vertex); | ||
| } | ||
|
|
||
| void GlobalVertexv3::clone_insert_vtx(GlobalVertex::VTXTYPE type, const Vertex* vertex) | ||
| { | ||
| auto it = _vtxs.find(type); | ||
| Vertex* clone = dynamic_cast<Vertex*>(vertex->CloneMe()); | ||
| if (it == _vtxs.end()) | ||
| { | ||
| VertexVector vector; | ||
| vector.push_back(clone); | ||
| _vtxs.insert(std::make_pair(type, vector)); | ||
| return; | ||
| } | ||
|
|
||
| it->second.push_back(clone); | ||
| } |
There was a problem hiding this comment.
Validate vertex pointers before storing/cloning.
insert_vtx/clone_insert_vtx accept raw pointers and later dereference/delete them (Reset/identify/get_*). A null pointer or failed CloneMe()/dynamic_cast will crash. Add guards and consider documenting the ownership contract in the header.
✅ Suggested fix
void GlobalVertexv3::insert_vtx(GlobalVertex::VTXTYPE type, const Vertex* vertex)
{
+ if (!vertex)
+ {
+ return;
+ }
auto it = _vtxs.find(type);
if (it == _vtxs.end())
{
@@
}
void GlobalVertexv3::clone_insert_vtx(GlobalVertex::VTXTYPE type, const Vertex* vertex)
{
+ if (!vertex)
+ {
+ return;
+ }
auto it = _vtxs.find(type);
Vertex* clone = dynamic_cast<Vertex*>(vertex->CloneMe());
+ if (!clone)
+ {
+ return;
+ }
if (it == _vtxs.end())
{| short int get_beam_crossing() const override | ||
| { | ||
| return rollover_from_unsignedint(_beamcrossing); | ||
| } | ||
| void set_beam_crossing(short int cross) override | ||
| { |
There was a problem hiding this comment.
Public API type change needs explicit compatibility notes
The virtual API changes from unsigned int to short int for beam crossing. Please confirm downstream overrides/call sites were updated and document any compatibility expectations (especially for external consumers).
As per coding guidelines: **/*.{h,hpp,hxx,hh}: Focus on API clarity/stability, ownership semantics (RAII), and avoiding raw new/delete. If interfaces change, ask for compatibility notes and any needed downstream updates.
| short int get_beam_crossing() const override | ||
| { | ||
| return rollover_from_unsignedint(_beamcrossing); | ||
| } | ||
| void set_beam_crossing(short int cross) override | ||
| { | ||
| if (cross == short_int_max) | ||
| { | ||
| _beamcrossing = std::numeric_limits<unsigned int>::max(); | ||
| return; | ||
| } | ||
|
|
||
| const short int cross_ro = rollover_short(cross); | ||
| _beamcrossing = static_cast<unsigned int>(cross_ro); | ||
| } |
There was a problem hiding this comment.
Beam‑crossing conversion drops negative values
rollover_short maps negative inputs into [0, short_int_max-1], and the getter treats that range as positive. Example: set_beam_crossing(-1) stores 32766, and get_beam_crossing() returns 32766 — the sign is lost. This defeats the goal of allowing negative crossings.
A bit‑preserving cast via unsigned short is invertible and keeps the sentinel behavior for short_int_max:
🔧 Suggested fix (invertible conversion)
short int get_beam_crossing() const override
{
return rollover_from_unsignedint(_beamcrossing);
}
void set_beam_crossing(short int cross) override
{
if (cross == short_int_max)
{
_beamcrossing = std::numeric_limits<unsigned int>::max();
return;
}
- const short int cross_ro = rollover_short(cross);
- _beamcrossing = static_cast<unsigned int>(cross_ro);
+ _beamcrossing = static_cast<unsigned int>(static_cast<unsigned short>(cross));
}
@@
- static short int rollover_short(short int cross)
- {
- if (cross == short_int_max) return short_int_max;
- if (cross >= 0) return cross;
-
- const int cross_ro = static_cast<int>(short_int_max) + static_cast<int>(cross); // cross negative
- return static_cast<short int>(cross_ro);
- }
-
static short int rollover_from_unsignedint(unsigned int cross)
{
// if unsigned int max, return short int max
if (cross == std::numeric_limits<unsigned int>::max())
{
return short_int_max;
}
- // Common case: [0, 32767]
- if (cross <= static_cast<unsigned int>(short_int_max))
- {
- return static_cast<short int>(cross);
- }
-
- const short int cross_ro = static_cast<short int>(static_cast<unsigned short>(cross));
- if (cross_ro >= 0) return cross_ro;
-
- return rollover_short(cross_ro);
+ return static_cast<short int>(static_cast<unsigned short>(cross));
}Also applies to: 88-118
| void SvtxVertex_v3::set_error(unsigned int i, unsigned int j, float value) | ||
| { | ||
| _err[covar_index(i, j)] = value; | ||
| return; | ||
| } | ||
|
|
||
| float SvtxVertex_v3::get_error(unsigned int i, unsigned int j) const | ||
| { | ||
| return _err[covar_index(i, j)]; | ||
| } | ||
|
|
||
| unsigned int SvtxVertex_v3::covar_index(unsigned int i, unsigned int j) const | ||
| { | ||
| if (i > j) | ||
| { | ||
| std::swap(i, j); | ||
| } | ||
| return i + 1 + (j + 1) * (j) / 2 - 1; | ||
| } |
There was a problem hiding this comment.
Add bounds checks for error-matrix indices.
set_error/get_error can index _err out of bounds when i or j ≥ 3, which is UB and a memory-safety risk. Guard the indices (and return NaN for invalid reads).
✅ Suggested fix
void SvtxVertex_v3::set_error(unsigned int i, unsigned int j, float value)
{
+ if (i >= 3 || j >= 3)
+ {
+ return;
+ }
_err[covar_index(i, j)] = value;
return;
}
float SvtxVertex_v3::get_error(unsigned int i, unsigned int j) const
{
+ if (i >= 3 || j >= 3)
+ {
+ return std::numeric_limits<float>::quiet_NaN();
+ }
return _err[covar_index(i, j)];
}| float get_position(unsigned int coor) const override { return _pos[coor]; } | ||
| void set_position(unsigned int coor, float xi) override { _pos[coor] = xi; } |
There was a problem hiding this comment.
Guard position index to prevent out-of-bounds access.
get_position/set_position index _pos[coor] directly; invalid coordinates produce UB. Please add bounds checks and return NaN (or ignore) for invalid indices.
✅ Suggested fix
- float get_position(unsigned int coor) const override { return _pos[coor]; }
- void set_position(unsigned int coor, float xi) override { _pos[coor] = xi; }
+ float get_position(unsigned int coor) const override
+ {
+ return (coor < 3) ? _pos[coor] : std::numeric_limits<float>::quiet_NaN();
+ }
+ void set_position(unsigned int coor, float xi) override
+ {
+ if (coor < 3) { _pos[coor] = xi; }
+ }
Build & test reportReport for commit d87e7150086359696e27c54635f97e2394d255ad:
Automatically generated by sPHENIX Jenkins continuous integration |
pinkenburg
merged commit b7268de
into
sPHENIX-Collaboration:master
4 participants
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
As stated in the title. (For example) PHSimpleVertexFinder assigns the vertex crossing using short int, while the SvtxVertex methods use unsigned int. This change also adds the function to the base class.
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)