Kf particle #9
Kf particle #9
Conversation
Fill ntuple for both matched and unmatched waveforms
…s_pool_evaluation_update Small update on TPOT event combining evaluation
if multiple hits are found on the same strip only the first one, timewise, is kept.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
add quality flag to total energy calc and correlations
fix: clang-format track fitter
…e into KFParticle
…Particle. Keeping the Pythia Trigger style for now
…ore-opening Run before opening
fixed crash when calopackets are empty
…t-add-OO add type 37: hijing O+O 0-15fm
…esoftware into new_tpc_transforms
…-drift-pythia8 this change fixes the tpc drift velocity being set to zero
Docstrings generation was requested by @pinkenburg. * sPHENIX-Collaboration#4111 (comment) The following files were modified: * `generators/PHPythia8/PHPythia8.cc`
Minor fixes to HepMC triggers
Add newline at the end of PHPythia8.cc file
…ion/coderabbitai/docstrings/63e54e6 📝 Add docstrings to `fix-tpc-drift-pythia8`
…-afterburner No flow afterburner
…transforms New tpc transforms
…e-cout Preserve cout
abort events with any mbd packet empty
New QA module to make cluster-state residual plots
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
generators/PHPythia8/PHPythia8.cc (1)
53-71: Guard against nullm_Pythia8whenPYTHIA8is unset.The constructor returns early if
PYTHIA8is missing, leavingm_Pythia8null;Init()/process_event()then dereference it and will crash. Add a guard (and returnABORTRUN) before first use so misconfiguration fails gracefully.🛠️ Proposed fix (add a guard in Init)
int PHPythia8::Init(PHCompositeNode *topNode) { + if (!m_Pythia8) + { + std::cout << PHWHERE << " ERROR: PYTHIA8 not set; Pythia8 not initialized." << std::endl; + return Fun4AllReturnCodes::ABORTRUN; + } if (!m_ConfigFileName.empty()) { read_config(m_ConfigFileName); }
🤖 Fix all issues with AI agents
In `@generators/Herwig/HepMCTrigger/HepMCParticleTrigger.cc`:
- Around line 79-107: The function currently treats a non-null but empty
PHHepMCGenEventMap (phg) as a successful event because the loop never runs and
good_event stays false, leading to returning EVENT_OK; fix this by explicitly
checking phg->empty() after retrieving phg and returning
Fun4AllReturnCodes::ABORTEVENT if it is empty so empty maps do not count as
valid events; update the logic around phg, the for-loop over
PHHepMCGenEventMap::ConstIter, and the n_good increment (leave n_good++ only
when a good_event was actually found via isGoodEvent on a real HepMC::GenEvent).
- Around line 344-351: The code uses std::map::contains (C++20) on
particle_types with pid which breaks builds on C++17; change the check to use
particle_types.find(pid) != particle_types.end() (or capture the iterator once)
and then increment the mapped value or insert a new entry accordingly, updating
the block that references particle_types and pid in HepMCParticleTrigger.cc so
it is C++17-compatible.
In `@offline/framework/ffamodules/HeadReco.cc`:
- Around line 87-93: When genevt->get_flow_psi_map() is empty the existing loop
that sets evtheader->set_FlowPsiN(n, genevt->get_flow_psi(n)) is skipped,
leaving stale values in EventHeader; add an else branch that iterates n = 1..6
and calls evtheader->set_FlowPsiN(n, 0.0) (or NaN if preferred) to reset all
harmonics, ensuring any previous FlowPsiN values are cleared when
genevt->get_flow_psi_map() is empty.
In `@offline/framework/fun4all/InputFileHandler.cc`:
- Around line 177-182: The current owner-only permission check rejects scripts
executable by group/other or allowed via the process's effective UID/GID;
replace the owner_exec bit test in InputFileHandler.cc (the block using
m_RunBeforeOpeningScript) with a real executable check using
access(m_RunBeforeOpeningScript.c_str(), X_OK) from <unistd.h> (include the
header), and keep the same error path (std::cout + return -1) but update the
message to indicate the script is not executable by the current process; ensure
you call access on the C-string form and check for a zero return to allow
execution.
- Around line 183-193: The code builds a shell command by concatenating
m_RunBeforeOpeningScript, m_OpeningArgs and entries of stringvec and then calls
gSystem->Exec, which allows command injection; modify InputFileHandler to avoid
shell interpretation by replacing gSystem->Exec(fullcmd.c_str()) with a direct
exec-style invocation (fork + execv or posix_spawn) that passes
m_RunBeforeOpeningScript as the program and a vector<char*> argv built from
m_RunBeforeOpeningScript, m_OpeningArgs split safely (or omitted) and each
element of stringvec as separate arguments; if exec-style replacement is not
possible, at minimum escape or strictly validate/quote each argument (validate
against a safe character whitelist) before concatenation and logging (refer to
m_RunBeforeOpeningScript, m_OpeningArgs, stringvec, gSystem->Exec and PHWHERE to
locate the affected code).
In `@offline/packages/jetbackground/DetermineTowerBackground.cc`:
- Around line 598-602: The code in DetermineTowerBackground::process_event
checks epmap and currently calls exit(-1) on failure; replace that exit call
with a framework-friendly return of Fun4AllReturnCodes::ABORTRUN so the routine
returns gracefully (i.e., change exit(-1) to return
Fun4AllReturnCodes::ABORTRUN), and ensure the translation unit can see
Fun4AllReturnCodes (add the appropriate include or fully qualify the symbol if
necessary); keep the diagnostic cout that logs _do_flow and epmap absence.
- Around line 953-957: The code in DetermineTowerBackground::process_event
aborts the program with exit(-1) when centinfo is null; replace that exit call
with returning Fun4AllReturnCodes::ABORTRUN to use the framework's consistent
error handling. Locate the null-check for centinfo (and the associated log line
that mentions _do_flow) and change the control flow to return
Fun4AllReturnCodes::ABORTRUN instead of calling exit, keeping the std::cout
error message intact.
- Around line 542-546: In DetermineTowerBackground::process_event where the code
returns raw -1 when truthinfo is null, replace the literal -1 with the framework
constant Fun4AllReturnCodes::ABORTRUN and ensure the translation unit can see
that symbol (add the appropriate include for Fun4AllReturnCodes if missing).
Update the return statement that currently returns -1 to return
Fun4AllReturnCodes::ABORTRUN and keep the existing diagnostic cout intact.
- Around line 87-91: The code currently calls exit(-1) when calibdir.empty();
replace that hard exit with a graceful return of Fun4AllReturnCodes::ABORTRUN
from the enclosing function (e.g., in DetermineTowerBackground.cc where the
calibdir.empty() check lives), and ensure the Fun4AllReturnCodes symbol is
visible (include the appropriate header or qualify the enum) so the function
returns the abort code instead of calling exit.
In `@offline/packages/mbd/MbdEvent.cc`:
- Around line 494-499: The code sets _mbdsig[feech].SetNSamples(_nsamples) but
skips SetXY when _nsamples > 30, leaving prior waveform data in _mbdsig[feech];
to fix, detect the out-of-range case (_nsamples <= 0 or _nsamples > 30) and mark
the channel empty by resetting the sample count and clearing waveform data
(e.g., call SetNSamples(0) and any available clear/reset method on
_mbdsig[feech]) instead of leaving the old samples; keep SetXY(m_samp[feech],
m_adc[feech]) only for the valid branch where 0 < _nsamples <= 30 so downstream
processing never sees stale data.
In `@offline/packages/mbd/MbdReco.cc`:
- Around line 104-108: The code leaves _evtnum unchanged when m_evtheader is
null, causing stale event numbers to persist; update the event-processing code
in MbdReco.cc (where m_evtheader, _evtnum and m_mbdevent->set_EventNumber are
used) to reset _evtnum at the start of each event (e.g., assign a sentinel like
-1 or 0) and then only overwrite it when m_evtheader!=nullptr, and ensure
m_mbdevent->set_EventNumber is always called with the sentinel when the header
is missing; apply the same change to the other block referenced (around the
124-140 region) so both places consistently clear and set the event number.
In `@offline/packages/micromegas/MicromegasClusterizer.cc`:
- Around line 223-275: The code stores iterator ranges into ranges using
TrkrHitSet::ConstRange but you built a LocalMap with custom comparator
(StripSortFtor), so TrkrHitSet::ConstRange iterators are incompatible; change
the range type to use LocalMap iterators (e.g., define a LocalRange type as
std::pair<LocalMap::const_iterator, LocalMap::const_iterator> or an alias using
LocalMap::const_iterator) and make the variable ranges (or range_list_t) use
that LocalRange, then emplace_back(begin, hit_it) and emplace_back(begin,
local_hitmap.end()) as before so the stored iterators match LocalMap.
In `@offline/packages/mvtx/MvtxClusterPruner.cc`:
- Around line 235-238: The current print in MvtxClusterPruner::End computes
double(m_cluster_counter_deleted) / m_cluster_counter_total which can divide by
zero; update the End method to guard the division by checking
m_cluster_counter_total > 0 (e.g., compute a local double fraction =
(m_cluster_counter_total > 0 ? double(m_cluster_counter_deleted) /
m_cluster_counter_total : 0.0) or use a sentinel like NAN/"N/A") and then print
m_cluster_counter_deleted, m_cluster_counter_total, and the safe fraction
variable instead of performing the raw division inline.
In `@offline/packages/tpc/LaserClusterizer.cc`:
- Around line 210-214: The code assumes regions[0] exists when copying into
clusHits; add a guard to avoid out-of-bounds access by checking clusHits (or
regions) before using regions[0] — e.g., if clusHits.empty() return/continue or
if regions.empty() return/continue — and ensure unvisited is populated only when
clusHits has elements so the subsequent while loop and access to regions[0] are
safe; update the block around clusHits.clear(), the loop that populates
clusHits, and any callers that rely on regions to handle the empty-case
consistently.
In `@offline/QA/Tracking/StateClusterResidualsQA.cc`:
- Around line 113-118: The loop that fills m_histograms_x, m_histograms_y, and
m_histograms_z uses dynamic_cast on hm->getHisto without checking for nullptr,
so add validation after retrieving each histogram pointer (from hm->getHisto and
after dynamic_cast to TH1*) and only push_back if the pointer is non-null; if
any retrieval fails, log a warning including cfg.name (or otherwise handle it)
and do not push into the vectors (or push a placeholder consistently) to keep
vector sizes aligned and avoid dereferencing nulls later in process_event (which
reads m_histograms_x/y/z).
- Around line 185-198: The code calls
geometry->getGlobalPosition(state->get_cluskey(), cluster) before verifying
cluster is non-null, risking a null dereference from cluster_map->findCluster;
change the control flow so you first call auto *cluster =
cluster_map->findCluster(state->get_cluskey()) and immediately check if
(cluster) before invoking geometry->getGlobalPosition or using cluster, and only
then compute glob, cluster_x/cluster_y/cluster_z and fill m_histograms_x[h],
m_histograms_y[h], m_histograms_z[h]; leave state->get_x(), get_y(), get_z() as
needed but ensure getGlobalPosition is not called with a null cluster.
In `@offline/QA/Tracking/StateClusterResidualsQA.h`:
- Line 127: The trailing `#endif` comment is wrong: replace the incorrect comment
"TRACKFITTINGQA_H" with the actual include guard name
"STATECLUSTERRESIDUALSQA_H" so the end of the header matches the opening guard;
update the comment after the `#endif` accordingly in StateClusterResidualsQA.h to
read `#endif` // STATECLUSTERRESIDUALSQA_H.
- Around line 56-101: All setter methods (setNMvtx, setNIntt, setNTpc,
setPhiRange, setEtaRange, setPtRange, setPositiveTracks, setNegativeTracks)
access m_pending.back() without ensuring m_pending is non-empty; update each
setter to guard against an empty m_pending by either creating a default pending
entry (e.g., if (m_pending.empty()) m_pending.emplace_back();) before using
back(), or by throwing/checking with a clear error message instructing the
caller to call addHistogram() first—apply the same fix to every setter so
m_pending.back() is always safe to access.
🧹 Nitpick comments (31)
generators/phhepmc/PHHepMCGenEventv1.cc (1)
113-114: Verify NaN propagation and prefer project logging overstd::cout.
Switching missingpsi_nfrom0.0toNaNis a behavior change; please confirm downstream callers (cuts, histogram fills, aggregations, serialization) explicitly handleNaNor check existence before use. Also consider routing this warning through the project logging facility to avoid noisy stdout from a const getter.offline/packages/mvtx/CylinderGeom_Mvtx.h (1)
14-20: Update stale comment in default constructor.Line 26 has a comment
/*int in_Nstaves*/that is now inconsistent with the parameter namein_N_staveson line 16.Suggested fix
//! default ctor to allow ROOT stream of this class. Implemented using c++11 feature of delegating constructors CylinderGeom_Mvtx() : CylinderGeom_Mvtx( /*int layer*/ 0, - /*int in_Nstaves*/ 0, + /*int in_N_staves*/ 0, /*double in_layer_nominal_radius*/ 3, /*double in_phistep*/ 0, /*double in_phitilt*/ 0, /*double in_phi0*/ 0)offline/packages/mvtx/CylinderGeom_Mvtx.cc (1)
124-134: Consider initializing variables for defensive coding.The split declarations are fine. The variables are set by
get_pixel_from_local_coordsbefore use, but initializing them to safe defaults would be slightly more defensive.Optional: Initialize variables
- int Ngridx; - int Ngridz; + int Ngridx = -1; + int Ngridz = -1;offline/packages/trackbase/AlignmentTransformation.cc (2)
280-291: Consider passinguse_module_tiltas a parameter tonewMakeTransform.The
use_module_tiltflag is set here and then read innewMakeTransform, creating implicit coupling through class member state. While this works correctly (the flag is reset at line 280 before each iteration), passing it as an explicit parameter would make the data flow clearer and the function more self-contained.Suggested approach
Add
bool use_module_tiltas a parameter tonewMakeTransformand pass it explicitly at the call site (line 295), similar to howsurveyis already passed.
491-498: Consider using Eigen's built-in subtraction for cleaner output.The nested loop manually computes element-wise differences. Eigen supports direct matrix subtraction which could simplify this.
♻️ Suggested simplification
std::cout << "overall - ideal " << std::endl; - for (int test = 0; test < transform.matrix().rows(); test++) - { - for (int test2 = 0; test2 < transform.matrix().cols(); test2++) - { - std::cout << transform(test, test2) - actstransform(test, test2) << ", "; - } - std::cout << std::endl; - } + std::cout << (transform.matrix() - actstransform.matrix()) << std::endl;offline/framework/fun4all/InputFileHandler.h (1)
36-40: API additions look good.The getter/setter pattern is consistent with the existing class interface. Consider adding brief documentation (inline or in a separate doc) clarifying:
- What
m_OpeningArgsformat is expected (space-separated? shell-escaped?)- That
RunBeforeOpeningexecutes a shell command (security implications for callers)offline/packages/jetbackground/DetermineTowerBackground.h (2)
16-16: Unused#include <array>.The
<array>header is included butstd::arraydoesn't appear to be used in this file. Consider removing it if not needed.Suggested fix
-#include <array>
41-45: Parameter should beconst std::string&.Taking a non-const lvalue reference prevents callers from passing string literals or temporaries directly. Use
const std::string&for flexibility.Suggested fix
- void SetOverwriteCaloV2(std::string &url) + void SetOverwriteCaloV2(const std::string &url) { m_overwrite_average_calo_v2 = true; m_overwrite_average_calo_v2_path = url; }offline/packages/jetbackground/DetermineTowerBackground.cc (1)
93-105: Consider usingstd::unique_ptrfor exception safety.Using a smart pointer would prevent memory leaks if an exception occurs during calibration loading.
Suggested fix
- cdbtree_calo_v2 = new CDBTTree(calibdir); - - - cdbtree_calo_v2->LoadCalibrations(); + auto cdbtree_calo_v2 = std::make_unique<CDBTTree>(calibdir); + cdbtree_calo_v2->LoadCalibrations(); _CENTRALITY_V2.assign(100,0); for (int icent = 0; icent < 100; icent++) { _CENTRALITY_V2[icent] = cdbtree_calo_v2->GetFloatValue(icent, "jet_calo_v2"); } - - delete cdbtree_calo_v2; + // unique_ptr automatically deletesoffline/QA/Tracking/StateClusterResidualsQA.cc (4)
32-33: Unused helper function.The
squaretemplate function is defined but never used in this file. Consider removing it to avoid dead code.
46-47: Markbegin()andend()as const member functions.For proper range-based for loop semantics and const-correctness, these methods should be
const.Suggested fix
- T const& begin() { return m_begin; } - T const& end() { return m_end; } + T const& begin() const { return m_begin; } + T const& end() const { return m_end; }
210-211: Consider graceful error handling instead of assert.
createHistos()is called (line 63) before the histogram manager null-check inInitRun(lines 102-111). IfgetHistoManager()returns nullptr, theassert(hm)will terminate the program rather than allowing a gracefulABORTRUN. Consider moving the histogram manager validation beforecreateHistos()or returning a status fromcreateHistos().
239-245: Unused variablehmin EndRun.The histogram manager is retrieved and asserted but never used. If no end-of-run processing is needed, consider removing the unused code to reduce confusion.
Suggested simplification
int StateClusterResidualsQA::EndRun(const int /*unused*/) { - auto *hm = QAHistManagerDef::getHistoManager(); - assert(hm); - return Fun4AllReturnCodes::EVENT_OK; }offline/QA/Tracking/StateClusterResidualsQA.h (2)
9-11: Unused include and inconsistent max value style.
<limits>is included but unused. Line 35 usesFLT_MAXfrom<cfloat>. Consider usingstd::numeric_limits<float>::max()for consistency with C++ idioms, or remove the unused<limits>include.
22-27: Micromegas cluster count selection not exposed.The implementation tracks micromegas cluster counts (in
StateClusterResidualsQA.ccline 141) butResidualHistConfigdoesn't includemin_micromegas_clusters/max_micromegas_clustersfields, and there's nosetNMicromegas()setter. If micromegas selection is needed in the future, consider adding these for consistency with the other detectors.offline/packages/tpc/TpcClusterMover.h (1)
30-30: LGTM!The
constqualifier aligns with the implementation inTpcClusterMover.ccwhich defines the method as const.Optional consistency note:
PHTpcClusterMover::get_circle_circle_intersectioninoffline/packages/trackreco/PHTpcClusterMover.cchas a similar signature withoutconst. Consider aligning both classes for consistency in a follow-up.offline/packages/tpc/Tpc3DClusterizer.cc (1)
523-528: Consider usingstd::numeric_limitsfor sentinel values.The magic numbers (6666, -1, 66666666.6) work but could be more expressive using
std::numeric_limits.♻️ Optional improvement
- int iphimin = 6666; - int iphimax = -1; - int ilaymin = 6666; - int ilaymax = -1; - float itmin = 66666666.6; - float itmax = -6666666666.6; + int iphimin = std::numeric_limits<int>::max(); + int iphimax = std::numeric_limits<int>::min(); + int ilaymin = std::numeric_limits<int>::max(); + int ilaymax = std::numeric_limits<int>::min(); + float itmin = std::numeric_limits<float>::max(); + float itmax = std::numeric_limits<float>::lowest();offline/packages/tpc/LaserClusterizer.cc (3)
833-836: Consider documenting the run number threshold.The magic number
66153determines which laser event check method to use. Consider adding a comment explaining the significance of this run number or extracting it as a named constant for maintainability.
1006-1007: Consider removing commented-out code.The commented line
// const auto &data(thread_pair.data);at line 1006 is unused. If no longer needed, consider removing it to keep the code clean.♻️ Proposed fix
- // const auto &data(thread_pair.data); - for (int index = 0; index < (int) thread_pair.data.cluster_vector.size(); ++index)
69-70: Remove unused global arrayslayerMinsandlayerMaxes.These arrays are declared but never referenced in this file or elsewhere in the codebase. They can be safely removed.
generators/Herwig/HepMCTrigger/HepMCJetTrigger.cc (2)
99-103: Minor typo in comment.The comment has a typo: "behavioro" should be "behaviour" or "behavior".
The physics logic is correct—skipping PDG IDs 12–18 excludes neutrinos (12, 14, 16) and charged leptons (13, 15) from jet clustering, which is standard practice since these particles typically don't contribute to hadronic jet energy.
📝 Fix typo
if (pd >= 12 && pd <= 18) { - continue; // keep jet in the expected behavioro + continue; // keep jet in the expected behavior }
130-133: LGTM, but consider making the eta cut configurable.The eta cut is reasonable for central detector acceptance. However, the value 1.1 is hardcoded. For consistency with
HepMCParticleTrigger(which uses configurable_theEtaHigh/_theEtaLow), consider adding a setter or constructor parameter for the jet eta cut.generators/Herwig/HepMCTrigger/Makefile.am (1)
45-46: Consider adding link test for the new library.
testexternalsonly links againstlibHepMCJetTrigger.la. For completeness, consider addinglibHepMCParticleTrigger.lato verify linkage, or create a separate test target.generators/Herwig/HepMCTrigger/HepMCParticleTrigger.h (2)
8-8: Unnecessary include.
fastjet/PseudoJet.hhis not used in this header. It can be removed to reduce compilation dependencies.♻️ Proposed fix
`#include` <fun4all/SubsysReco.h> -#include <fastjet/PseudoJet.hh> - `#include` <cmath>
85-90: Inconsistent default for_thePtLow.The header initializes
_thePtLow{-999.9}but the constructor sets it to0. Since pT is always non-negative, the header default is misleading. Align the defaults for clarity.♻️ Proposed fix
- float _thePtLow{-999.9}; + float _thePtLow{0.0};generators/Herwig/HepMCTrigger/HepMCParticleTrigger.cc (2)
12-16: Remove unused FastJet includes.These FastJet headers are not used in this file. They appear to be copy-paste artifacts from
HepMCJetTrigger.cc.♻️ Proposed fix
`#include` <phool/getClass.h> -#include <fastjet/JetDefinition.hh> - `#include` <HepMC/GenEvent.h> -#include <fastjet/PseudoJet.hh> `#include` <map>
22-65: Constructor initializer list has unusual formatting.The blank lines within the member initializer list (e.g., lines 35–36, 40–41) are unconventional. Consider running clang-format for consistency with the rest of the codebase.
offline/packages/mbd/MbdSig.h (1)
35-35: Make the accessorconst(and optionallynoexcept).This accessor doesn’t mutate state; marking it
constimproves const-correctness and usage on const instances.♻️ Proposed change
- int GetNSamples() { return _nsamples; } + int GetNSamples() const noexcept { return _nsamples; }offline/packages/tpc/LaserEventIdentifier.cc (1)
141-148: Consider extracting magic number and handling BCO rollover at counter boundaries.Two observations on this existing logic:
Magic number:
350.0/30*16(~187) should be a named constant with a comment explaining its derivation (appears to be a timing threshold in BCO units).BCO rollover: When BCO counters wrap around, if
prev_BCO > gl1pkt->getBCO(), the unsigned subtraction produces a very large value due to underflow, causing the pileup condition to be false when it should potentially be true. Consider using a helper function or modular arithmetic to safely compare BCO values across rollover boundaries.offline/QA/Tracking/MicromegasClusterQA.h (1)
54-58: Guard invalid sample window configurationsIf
m_sample_min >= m_sample_max, the new filtering logic will drop every cluster. Consider validating (or at least warning) on invalid ranges in setters orInitRunto avoid silent misconfiguration.Also applies to: 108-112
offline/QA/Tracking/MicromegasClusterQA.cc (1)
168-173: Validate sample window to avoid dropping all clustersIf
m_sample_min >= m_sample_max, the predicate is always false and every cluster is skipped. Consider guarding or warning on invalid ranges (in setters orInitRun) so misconfiguration is caught early.
| bool good_event{false}; | ||
| PHHepMCGenEventMap* phg = findNode::getClass<PHHepMCGenEventMap>(topNode, "PHHepMCGenEventMap"); | ||
| if (!phg) | ||
| { | ||
| return Fun4AllReturnCodes::ABORTEVENT; | ||
| } | ||
| for (PHHepMCGenEventMap::ConstIter eventIter = phg->begin(); eventIter != phg->end(); ++eventIter) | ||
| { | ||
| PHHepMCGenEvent* hepev = eventIter->second; | ||
| if (!hepev) | ||
| { | ||
| return Fun4AllReturnCodes::ABORTEVENT; | ||
| } | ||
| HepMC::GenEvent* ev = hepev->getEvent(); | ||
| if (!ev) | ||
| { | ||
| return Fun4AllReturnCodes::ABORTEVENT; | ||
| } | ||
| good_event = isGoodEvent(ev); | ||
| if (!good_event) | ||
| { | ||
| return Fun4AllReturnCodes::ABORTEVENT; | ||
| } | ||
| } | ||
| if (good_event) | ||
| { | ||
| n_good++; | ||
| } | ||
| return Fun4AllReturnCodes::EVENT_OK; |
There was a problem hiding this comment.
Empty event map returns EVENT_OK with good_event=false.
If PHHepMCGenEventMap is non-null but empty, the for-loop never executes, good_event remains false, and the function returns EVENT_OK without incrementing n_good. This may not be the intended behavior.
🐛 Proposed fix
PHHepMCGenEventMap* phg = findNode::getClass<PHHepMCGenEventMap>(topNode, "PHHepMCGenEventMap");
if (!phg)
{
return Fun4AllReturnCodes::ABORTEVENT;
}
+ if (phg->size() == 0)
+ {
+ return Fun4AllReturnCodes::ABORTEVENT;
+ }
for (PHHepMCGenEventMap::ConstIter eventIter = phg->begin(); eventIter != phg->end(); ++eventIter)📝 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.
| bool good_event{false}; | |
| PHHepMCGenEventMap* phg = findNode::getClass<PHHepMCGenEventMap>(topNode, "PHHepMCGenEventMap"); | |
| if (!phg) | |
| { | |
| return Fun4AllReturnCodes::ABORTEVENT; | |
| } | |
| for (PHHepMCGenEventMap::ConstIter eventIter = phg->begin(); eventIter != phg->end(); ++eventIter) | |
| { | |
| PHHepMCGenEvent* hepev = eventIter->second; | |
| if (!hepev) | |
| { | |
| return Fun4AllReturnCodes::ABORTEVENT; | |
| } | |
| HepMC::GenEvent* ev = hepev->getEvent(); | |
| if (!ev) | |
| { | |
| return Fun4AllReturnCodes::ABORTEVENT; | |
| } | |
| good_event = isGoodEvent(ev); | |
| if (!good_event) | |
| { | |
| return Fun4AllReturnCodes::ABORTEVENT; | |
| } | |
| } | |
| if (good_event) | |
| { | |
| n_good++; | |
| } | |
| return Fun4AllReturnCodes::EVENT_OK; | |
| bool good_event{false}; | |
| PHHepMCGenEventMap* phg = findNode::getClass<PHHepMCGenEventMap>(topNode, "PHHepMCGenEventMap"); | |
| if (!phg) | |
| { | |
| return Fun4AllReturnCodes::ABORTEVENT; | |
| } | |
| if (phg->size() == 0) | |
| { | |
| return Fun4AllReturnCodes::ABORTEVENT; | |
| } | |
| for (PHHepMCGenEventMap::ConstIter eventIter = phg->begin(); eventIter != phg->end(); ++eventIter) | |
| { | |
| PHHepMCGenEvent* hepev = eventIter->second; | |
| if (!hepev) | |
| { | |
| return Fun4AllReturnCodes::ABORTEVENT; | |
| } | |
| HepMC::GenEvent* ev = hepev->getEvent(); | |
| if (!ev) | |
| { | |
| return Fun4AllReturnCodes::ABORTEVENT; | |
| } | |
| good_event = isGoodEvent(ev); | |
| if (!good_event) | |
| { | |
| return Fun4AllReturnCodes::ABORTEVENT; | |
| } | |
| } | |
| if (good_event) | |
| { | |
| n_good++; | |
| } | |
| return Fun4AllReturnCodes::EVENT_OK; |
🤖 Prompt for AI Agents
In `@generators/Herwig/HepMCTrigger/HepMCParticleTrigger.cc` around lines 79 -
107, The function currently treats a non-null but empty PHHepMCGenEventMap (phg)
as a successful event because the loop never runs and good_event stays false,
leading to returning EVENT_OK; fix this by explicitly checking phg->empty()
after retrieving phg and returning Fun4AllReturnCodes::ABORTEVENT if it is empty
so empty maps do not count as valid events; update the logic around phg, the
for-loop over PHHepMCGenEventMap::ConstIter, and the n_good increment (leave
n_good++ only when a good_event was actually found via isGoodEvent on a real
HepMC::GenEvent).
| if (particle_types.contains(pid)) | ||
| { | ||
| particle_types[pid]++; | ||
| } | ||
| else | ||
| { | ||
| particle_types[pid] = 1; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for build configuration files
find . -maxdepth 3 -type f \( -name "CMakeLists.txt" -o -name "configure.ac" -o -name "Makefile.am" \) | head -20Repository: sPHENIX-Test/coresoftware
Length of output: 852
🏁 Script executed:
# Search for C++ standard declarations across the repository
rg -i 'c\+\+20|c\+\+2a|cxx_std_20|CXX_STANDARD.*20|std=c\+\+20' --type-list | head -5
rg -i 'c\+\+20|c\+\+2a|cxx_std_20|CXX_STANDARD.*20|std=c\+\+20' -g "*.txt" -g "*.ac" -g "*.am" -g "*.cmake" 2>/dev/null | head -20Repository: sPHENIX-Test/coresoftware
Length of output: 458
🏁 Script executed:
# Look for any C++ standard specifications
rg -i 'CXX_STANDARD|c\+\+17|c\+\+11|c\+\+14' -g "CMakeLists.txt" -g "configure.ac" -g "Makefile.am" 2>/dev/null | head -30Repository: sPHENIX-Test/coresoftware
Length of output: 471
🏁 Script executed:
# Check the specific directory and parent directories for build files
ls -la generators/Herwig/HepMCTrigger/ 2>/dev/null | head -20
ls -la generators/Herwig/ 2>/dev/null | head -20
ls -la generators/ 2>/dev/null | grep -i cmakeRepository: sPHENIX-Test/coresoftware
Length of output: 816
🏁 Script executed:
# Check the Makefile.am in the HepMCTrigger directory
cat generators/Herwig/HepMCTrigger/Makefile.amRepository: sPHENIX-Test/coresoftware
Length of output: 1134
🏁 Script executed:
# Check the configure.ac in the HepMCTrigger directory
cat generators/Herwig/HepMCTrigger/configure.acRepository: sPHENIX-Test/coresoftware
Length of output: 440
🏁 Script executed:
# Check if there's a parent configure.ac in generators/Herwig
find generators/Herwig -maxdepth 1 -name "configure.ac" -o -name "Makefile.am" 2>/dev/nullRepository: sPHENIX-Test/coresoftware
Length of output: 51
🏁 Script executed:
# Search for CXX_STANDARD or C++ flag declarations in the entire generators directory
rg -i 'CXX_STANDARD|AM_CXXFLAGS|std=' generators/Herwig/HepMCTrigger/ --type=make --type=cmake 2>/dev/null || echo "No matches found"Repository: sPHENIX-Test/coresoftware
Length of output: 85
🏁 Script executed:
# Check if there's a root configure.ac or global C++ standard configuration
head -50 configure.ac 2>/dev/null || echo "No root configure.ac"Repository: sPHENIX-Test/coresoftware
Length of output: 89
🏁 Script executed:
# Search for any other C++20 features in the HepMCTrigger files
rg 'contains\(|spaceship|concepts|requires|co_await' generators/Herwig/HepMCTrigger/ --type=cc --type=hRepository: sPHENIX-Test/coresoftware
Length of output: 95
🏁 Script executed:
# Check if there's a toplevel CMakeLists.txt
ls -la CMakeLists.txt 2>/dev/null || echo "No CMakeLists.txt at root"Repository: sPHENIX-Test/coresoftware
Length of output: 94
🏁 Script executed:
# Check the specific line in HepMCParticleTrigger.cc to confirm context
sed -n '340,355p' generators/Herwig/HepMCTrigger/HepMCParticleTrigger.ccRepository: sPHENIX-Test/coresoftware
Length of output: 366
Replace std::map::contains() with C++17-compatible find() method.
The build configuration for this module does not explicitly require C++20. The code uses std::map::contains() (C++20) on line 344, which will fail to compile on systems defaulting to C++17 or earlier. Replace with find() for compatibility:
C++17-compatible fix
- if (particle_types.contains(pid))
+ if (particle_types.find(pid) != particle_types.end())
{
particle_types[pid]++;
}📝 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.
| if (particle_types.contains(pid)) | |
| { | |
| particle_types[pid]++; | |
| } | |
| else | |
| { | |
| particle_types[pid] = 1; | |
| } | |
| if (particle_types.find(pid) != particle_types.end()) | |
| { | |
| particle_types[pid]++; | |
| } | |
| else | |
| { | |
| particle_types[pid] = 1; | |
| } |
🤖 Prompt for AI Agents
In `@generators/Herwig/HepMCTrigger/HepMCParticleTrigger.cc` around lines 344 -
351, The code uses std::map::contains (C++20) on particle_types with pid which
breaks builds on C++17; change the check to use particle_types.find(pid) !=
particle_types.end() (or capture the iterator once) and then increment the
mapped value or insert a new entry accordingly, updating the block that
references particle_types and pid in HepMCParticleTrigger.cc so it is
C++17-compatible.
| if (! genevt->get_flow_psi_map().empty()) | ||
| { | ||
| for (unsigned int n = 1; n <= 6; ++n) | ||
| { | ||
| evtheader->set_FlowPsiN(n, genevt->get_flow_psi(n)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Explore the file structure and context around lines 87-93
head -100 offline/framework/ffamodules/HeadReco.cc | tail -30Repository: sPHENIX-Test/coresoftware
Length of output: 1006
🏁 Script executed:
# Get broader context of the HeadReco.cc file
wc -l offline/framework/ffamodules/HeadReco.ccRepository: sPHENIX-Test/coresoftware
Length of output: 113
🏁 Script executed:
# Look for EventHeader creation/instantiation patterns
rg -n "EventHeader\s*\*|new\s+EventHeader|evtheader\s*=" offline/framework/ffamodules/HeadReco.cc -A 2 -B 2Repository: sPHENIX-Test/coresoftware
Length of output: 606
🏁 Script executed:
# Search for reset/initialization patterns in the file
rg -n "evtheader->set_|evtheader->reset|FlowPsiN" offline/framework/ffamodules/HeadReco.cc -B 1Repository: sPHENIX-Test/coresoftware
Length of output: 679
🏁 Script executed:
# Check the EventHeader class definition to understand FlowPsiN
fd -e h -e hh | xargs rg -l "class EventHeader|FlowPsiN" | head -5Repository: sPHENIX-Test/coresoftware
Length of output: 312
🏁 Script executed:
# Check EventHeaderv1 class definition for FlowPsiN
cat offline/framework/ffaobjects/EventHeaderv1.hRepository: sPHENIX-Test/coresoftware
Length of output: 1796
🏁 Script executed:
# Also check v2 since the code might use that
cat offline/framework/ffaobjects/EventHeaderv2.hRepository: sPHENIX-Test/coresoftware
Length of output: 1250
🏁 Script executed:
# Check if there's any initialization of EventHeader at the start of process_event
sed -n '65,110p' offline/framework/ffamodules/HeadReco.ccRepository: sPHENIX-Test/coresoftware
Length of output: 1463
🏁 Script executed:
# Search for set_FlowPsiN implementation
rg -n "set_FlowPsiN|get_FlowPsiN" --type cpp --type h -B 3 -A 5Repository: sPHENIX-Test/coresoftware
Length of output: 1807
🏁 Script executed:
# Check if Reset() is called anywhere in HeadReco.cc or at the start of process_event
rg -n "Reset()" offline/framework/ffamodules/HeadReco.ccRepository: sPHENIX-Test/coresoftware
Length of output: 51
🏁 Script executed:
# Search for EventHeader base class to find where FlowPsiN is implemented
cat offline/framework/ffaobjects/EventHeader.h | head -150Repository: sPHENIX-Test/coresoftware
Length of output: 3036
🏁 Script executed:
# Check EventHeaderv1 Reset implementation
fd -e cc -e cpp | xargs rg -l "EventHeaderv1::Reset" | head -1 | xargs catRepository: sPHENIX-Test/coresoftware
Length of output: 669
🏁 Script executed:
# Also look for EventHeaderv2::Reset if it exists
fd -e cc -e cpp | xargs rg -l "EventHeaderv2::Reset" | head -1 | xargs catRepository: sPHENIX-Test/coresoftware
Length of output: 669
🏁 Script executed:
# Check if there's any pattern where FlowPsiN or "psi_" values are reset/cleared elsewhere
rg -n "psi_|FlowPsiN" --type cpp --type h -B 2 -A 2 | head -100Repository: sPHENIX-Test/coresoftware
Length of output: 9039
🏁 Script executed:
# Find EventHeaderv1.cc and check Reset() implementation
fd -e cc -e cpp | xargs rg -l "EventHeaderv1::Reset" | head -1 | xargs catRepository: sPHENIX-Test/coresoftware
Length of output: 2090
🏁 Script executed:
# Also search for EventHeaderv1::Reset in all files
rg -n "EventHeaderv1::Reset\|void.*Reset\(\)" offline/framework/ffaobjects/EventHeaderv1.cc -A 10Repository: sPHENIX-Test/coresoftware
Length of output: 51
Reset FlowPsiN values when the flow_psi_map is empty to prevent stale data leakage.
The EventHeader object persists across events in a run and is never reset between them. When get_flow_psi_map() is empty, the code skips updating FlowPsiN, leaving old values from the previous event in the object. This causes data corruption for events without flow information. Add an else clause to reset all FlowPsiN harmonics to a known default (0.0 or NaN) when the map is empty.
Suggested fix
- if (! genevt->get_flow_psi_map().empty())
- {
- for (unsigned int n = 1; n <= 6; ++n)
- {
- evtheader->set_FlowPsiN(n, genevt->get_flow_psi(n));
- }
- }
+ if (!genevt->get_flow_psi_map().empty())
+ {
+ for (unsigned int n = 1; n <= 6; ++n)
+ {
+ evtheader->set_FlowPsiN(n, genevt->get_flow_psi(n));
+ }
+ }
+ else
+ {
+ for (unsigned int n = 1; n <= 6; ++n)
+ {
+ evtheader->set_FlowPsiN(n, 0.0);
+ }
+ }📝 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.
| if (! genevt->get_flow_psi_map().empty()) | |
| { | |
| for (unsigned int n = 1; n <= 6; ++n) | |
| { | |
| evtheader->set_FlowPsiN(n, genevt->get_flow_psi(n)); | |
| } | |
| } | |
| if (!genevt->get_flow_psi_map().empty()) | |
| { | |
| for (unsigned int n = 1; n <= 6; ++n) | |
| { | |
| evtheader->set_FlowPsiN(n, genevt->get_flow_psi(n)); | |
| } | |
| } | |
| else | |
| { | |
| for (unsigned int n = 1; n <= 6; ++n) | |
| { | |
| evtheader->set_FlowPsiN(n, 0.0); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@offline/framework/ffamodules/HeadReco.cc` around lines 87 - 93, When
genevt->get_flow_psi_map() is empty the existing loop that sets
evtheader->set_FlowPsiN(n, genevt->get_flow_psi(n)) is skipped, leaving stale
values in EventHeader; add an else branch that iterates n = 1..6 and calls
evtheader->set_FlowPsiN(n, 0.0) (or NaN if preferred) to reset all harmonics,
ensuring any previous FlowPsiN values are cleared when
genevt->get_flow_psi_map() is empty.
| if (!((std::filesystem::status(m_RunBeforeOpeningScript).permissions() & std::filesystem::perms::owner_exec) == std::filesystem::perms::owner_exec)) | ||
| { | ||
| std::cout << PHWHERE << "RunBeforeOpeningScript script " | ||
| << m_RunBeforeOpeningScript << " is not owner executable" << std::endl; | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
Permission check may be too restrictive.
Only owner_exec is verified. If the script is owned by another user (e.g., root) but has group or world execute permissions, this check will incorrectly reject a valid executable script.
Consider using access(path, X_OK) from <unistd.h> which checks if the current process can execute the file, accounting for actual user/group permissions.
🔧 Suggested fix using access()
+#include <unistd.h>
// ...
- if (!((std::filesystem::status(m_RunBeforeOpeningScript).permissions() & std::filesystem::perms::owner_exec) == std::filesystem::perms::owner_exec))
+ if (access(m_RunBeforeOpeningScript.c_str(), X_OK) != 0)
{
std::cout << PHWHERE << "RunBeforeOpeningScript script "
<< m_RunBeforeOpeningScript << " is not executable" << std::endl;
return -1;
}📝 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.
| if (!((std::filesystem::status(m_RunBeforeOpeningScript).permissions() & std::filesystem::perms::owner_exec) == std::filesystem::perms::owner_exec)) | |
| { | |
| std::cout << PHWHERE << "RunBeforeOpeningScript script " | |
| << m_RunBeforeOpeningScript << " is not owner executable" << std::endl; | |
| return -1; | |
| } | |
| if (access(m_RunBeforeOpeningScript.c_str(), X_OK) != 0) | |
| { | |
| std::cout << PHWHERE << "RunBeforeOpeningScript script " | |
| << m_RunBeforeOpeningScript << " is not executable" << std::endl; | |
| return -1; | |
| } |
🤖 Prompt for AI Agents
In `@offline/framework/fun4all/InputFileHandler.cc` around lines 177 - 182, The
current owner-only permission check rejects scripts executable by group/other or
allowed via the process's effective UID/GID; replace the owner_exec bit test in
InputFileHandler.cc (the block using m_RunBeforeOpeningScript) with a real
executable check using access(m_RunBeforeOpeningScript.c_str(), X_OK) from
<unistd.h> (include the header), and keep the same error path (std::cout +
return -1) but update the message to indicate the script is not executable by
the current process; ensure you call access on the C-string form and check for a
zero return to allow execution.
| std::string fullcmd = m_RunBeforeOpeningScript + " " + m_OpeningArgs; | ||
| for (const auto& iter : stringvec) | ||
| { | ||
| fullcmd += " " + iter; | ||
| } | ||
|
|
||
| if (m_Verbosity > 1) | ||
| { | ||
| std::cout << PHWHERE << " running " << fullcmd << std::endl; | ||
| } | ||
| unsigned int iret = gSystem->Exec(fullcmd.c_str()); |
There was a problem hiding this comment.
Command injection vulnerability via unsanitized arguments.
Filenames and m_OpeningArgs are concatenated directly into a shell command without escaping. Filenames containing shell metacharacters (;, $(), backticks, |, etc.) could execute arbitrary code.
Consider:
- Quoting each argument (though this doesn't fully prevent injection)
- Using an exec-style API that passes arguments as a vector (avoiding shell interpretation)
- At minimum, validating that arguments don't contain dangerous characters
🛡️ Minimal mitigation: quote arguments
- std::string fullcmd = m_RunBeforeOpeningScript + " " + m_OpeningArgs;
+ std::string fullcmd = "'" + m_RunBeforeOpeningScript + "'";
+ if (!m_OpeningArgs.empty())
+ {
+ fullcmd += " " + m_OpeningArgs; // User-provided args - document they must be safe
+ }
for (const auto& iter : stringvec)
{
- fullcmd += " " + iter;
+ // Escape single quotes within the filename and wrap in single quotes
+ std::string escaped = iter;
+ size_t pos = 0;
+ while ((pos = escaped.find('\'', pos)) != std::string::npos)
+ {
+ escaped.replace(pos, 1, "'\\''");
+ pos += 4;
+ }
+ fullcmd += " '" + escaped + "'";
}Note: The safest approach would be to use fork/execv or a similar API that doesn't invoke a shell, passing arguments as separate strings.
📝 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.
| std::string fullcmd = m_RunBeforeOpeningScript + " " + m_OpeningArgs; | |
| for (const auto& iter : stringvec) | |
| { | |
| fullcmd += " " + iter; | |
| } | |
| if (m_Verbosity > 1) | |
| { | |
| std::cout << PHWHERE << " running " << fullcmd << std::endl; | |
| } | |
| unsigned int iret = gSystem->Exec(fullcmd.c_str()); | |
| std::string fullcmd = "'" + m_RunBeforeOpeningScript + "'"; | |
| if (!m_OpeningArgs.empty()) | |
| { | |
| fullcmd += " " + m_OpeningArgs; // User-provided args - document they must be safe | |
| } | |
| for (const auto& iter : stringvec) | |
| { | |
| // Escape single quotes within the filename and wrap in single quotes | |
| std::string escaped = iter; | |
| size_t pos = 0; | |
| while ((pos = escaped.find('\'', pos)) != std::string::npos) | |
| { | |
| escaped.replace(pos, 1, "'\\''"); | |
| pos += 4; | |
| } | |
| fullcmd += " '" + escaped + "'"; | |
| } | |
| if (m_Verbosity > 1) | |
| { | |
| std::cout << PHWHERE << " running " << fullcmd << std::endl; | |
| } | |
| unsigned int iret = gSystem->Exec(fullcmd.c_str()); |
🤖 Prompt for AI Agents
In `@offline/framework/fun4all/InputFileHandler.cc` around lines 183 - 193, The
code builds a shell command by concatenating m_RunBeforeOpeningScript,
m_OpeningArgs and entries of stringvec and then calls gSystem->Exec, which
allows command injection; modify InputFileHandler to avoid shell interpretation
by replacing gSystem->Exec(fullcmd.c_str()) with a direct exec-style invocation
(fork + execv or posix_spawn) that passes m_RunBeforeOpeningScript as the
program and a vector<char*> argv built from m_RunBeforeOpeningScript,
m_OpeningArgs split safely (or omitted) and each element of stringvec as
separate arguments; if exec-style replacement is not possible, at minimum escape
or strictly validate/quote each argument (validate against a safe character
whitelist) before concatenation and logging (refer to m_RunBeforeOpeningScript,
m_OpeningArgs, stringvec, gSystem->Exec and PHWHERE to locate the affected
code).
| clusHits.clear(); | ||
| for(auto hit : regions[0]) | ||
| for (auto hit : regions[0]) | ||
| { | ||
| clusHits.push_back(hit); | ||
| } |
There was a problem hiding this comment.
Potential out-of-bounds access if clusHits is empty.
If clusHits is empty when passed to this function, unvisited will be empty, the while loop will not execute, and regions will remain empty. Accessing regions[0] on line 211 would then cause undefined behavior.
Consider adding a guard:
🐛 Proposed fix
clusHits.clear();
+ if (regions.empty())
+ {
+ return;
+ }
for (auto hit : regions[0])
{
clusHits.push_back(hit);
}📝 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.
| clusHits.clear(); | |
| for(auto hit : regions[0]) | |
| for (auto hit : regions[0]) | |
| { | |
| clusHits.push_back(hit); | |
| } | |
| clusHits.clear(); | |
| if (regions.empty()) | |
| { | |
| return; | |
| } | |
| for (auto hit : regions[0]) | |
| { | |
| clusHits.push_back(hit); | |
| } |
🤖 Prompt for AI Agents
In `@offline/packages/tpc/LaserClusterizer.cc` around lines 210 - 214, The code
assumes regions[0] exists when copying into clusHits; add a guard to avoid
out-of-bounds access by checking clusHits (or regions) before using regions[0] —
e.g., if clusHits.empty() return/continue or if regions.empty() return/continue
— and ensure unvisited is populated only when clusHits has elements so the
subsequent while loop and access to regions[0] are safe; update the block around
clusHits.clear(), the loop that populates clusHits, and any callers that rely on
regions to handle the empty-case consistently.
| for (const auto& cfg : m_pending) | ||
| { | ||
| m_histograms_x.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_x")))); | ||
| m_histograms_y.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_y")))); | ||
| m_histograms_z.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_z")))); | ||
| } |
There was a problem hiding this comment.
Missing null-check after histogram retrieval.
If getHisto returns nullptr or the dynamic_cast fails, nullptr will be stored in the histogram vectors. Later in process_event, these will be dereferenced (lines 195-197) causing a crash. Consider validating the histograms exist.
Suggested validation
for (const auto& cfg : m_pending)
{
- m_histograms_x.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_x"))));
- m_histograms_y.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_y"))));
- m_histograms_z.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_z"))));
+ auto* h_x = dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_x")));
+ auto* h_y = dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_y")));
+ auto* h_z = dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_z")));
+ if (!h_x || !h_y || !h_z)
+ {
+ std::cout << PHWHERE << "\n"
+ << "\tCould not retrieve histograms for config: " << cfg.name << "\n"
+ << "\tAborting\n" << std::endl;
+ return Fun4AllReturnCodes::ABORTRUN;
+ }
+ m_histograms_x.push_back(h_x);
+ m_histograms_y.push_back(h_y);
+ m_histograms_z.push_back(h_z);
}📝 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.
| for (const auto& cfg : m_pending) | |
| { | |
| m_histograms_x.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_x")))); | |
| m_histograms_y.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_y")))); | |
| m_histograms_z.push_back(dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_z")))); | |
| } | |
| for (const auto& cfg : m_pending) | |
| { | |
| auto* h_x = dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_x"))); | |
| auto* h_y = dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_y"))); | |
| auto* h_z = dynamic_cast<TH1 *>(hm->getHisto(std::string(cfg.name + "_z"))); | |
| if (!h_x || !h_y || !h_z) | |
| { | |
| std::cout << PHWHERE << "\n" | |
| << "\tCould not retrieve histograms for config: " << cfg.name << "\n" | |
| << "\tAborting\n" << std::endl; | |
| return Fun4AllReturnCodes::ABORTRUN; | |
| } | |
| m_histograms_x.push_back(h_x); | |
| m_histograms_y.push_back(h_y); | |
| m_histograms_z.push_back(h_z); | |
| } |
🤖 Prompt for AI Agents
In `@offline/QA/Tracking/StateClusterResidualsQA.cc` around lines 113 - 118, The
loop that fills m_histograms_x, m_histograms_y, and m_histograms_z uses
dynamic_cast on hm->getHisto without checking for nullptr, so add validation
after retrieving each histogram pointer (from hm->getHisto and after
dynamic_cast to TH1*) and only push_back if the pointer is non-null; if any
retrieval fails, log a warning including cfg.name (or otherwise handle it) and
do not push into the vectors (or push a placeholder consistently) to keep vector
sizes aligned and avoid dereferencing nulls later in process_event (which reads
m_histograms_x/y/z).
| auto *cluster = cluster_map->findCluster(state->get_cluskey()); | ||
| float state_x = state->get_x(); | ||
| float state_y = state->get_y(); | ||
| float state_z = state->get_z(); | ||
| Acts::Vector3 glob = geometry->getGlobalPosition(state->get_cluskey(), cluster); | ||
| float cluster_x = glob.x(); | ||
| float cluster_y = glob.y(); | ||
| float cluster_z = glob.z(); | ||
| if (cluster) | ||
| { | ||
| m_histograms_x[h]->Fill(state_x - cluster_x); | ||
| m_histograms_y[h]->Fill(state_y - cluster_y); | ||
| m_histograms_z[h]->Fill(state_z - cluster_z); | ||
| } |
There was a problem hiding this comment.
Null pointer dereference: cluster used before null check.
On line 189, cluster is passed to getGlobalPosition before validating it's non-null on line 193. If findCluster returns nullptr, this will cause undefined behavior or a crash.
Proposed fix: check cluster before use
auto *cluster = cluster_map->findCluster(state->get_cluskey());
+ if (!cluster) { continue; }
float state_x = state->get_x();
float state_y = state->get_y();
float state_z = state->get_z();
Acts::Vector3 glob = geometry->getGlobalPosition(state->get_cluskey(), cluster);
float cluster_x = glob.x();
float cluster_y = glob.y();
float cluster_z = glob.z();
- if (cluster)
- {
- m_histograms_x[h]->Fill(state_x - cluster_x);
- m_histograms_y[h]->Fill(state_y - cluster_y);
- m_histograms_z[h]->Fill(state_z - cluster_z);
- }
+ m_histograms_x[h]->Fill(state_x - cluster_x);
+ m_histograms_y[h]->Fill(state_y - cluster_y);
+ m_histograms_z[h]->Fill(state_z - cluster_z);📝 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.
| auto *cluster = cluster_map->findCluster(state->get_cluskey()); | |
| float state_x = state->get_x(); | |
| float state_y = state->get_y(); | |
| float state_z = state->get_z(); | |
| Acts::Vector3 glob = geometry->getGlobalPosition(state->get_cluskey(), cluster); | |
| float cluster_x = glob.x(); | |
| float cluster_y = glob.y(); | |
| float cluster_z = glob.z(); | |
| if (cluster) | |
| { | |
| m_histograms_x[h]->Fill(state_x - cluster_x); | |
| m_histograms_y[h]->Fill(state_y - cluster_y); | |
| m_histograms_z[h]->Fill(state_z - cluster_z); | |
| } | |
| auto *cluster = cluster_map->findCluster(state->get_cluskey()); | |
| if (!cluster) { continue; } | |
| float state_x = state->get_x(); | |
| float state_y = state->get_y(); | |
| float state_z = state->get_z(); | |
| Acts::Vector3 glob = geometry->getGlobalPosition(state->get_cluskey(), cluster); | |
| float cluster_x = glob.x(); | |
| float cluster_y = glob.y(); | |
| float cluster_z = glob.z(); | |
| m_histograms_x[h]->Fill(state_x - cluster_x); | |
| m_histograms_y[h]->Fill(state_y - cluster_y); | |
| m_histograms_z[h]->Fill(state_z - cluster_z); |
🤖 Prompt for AI Agents
In `@offline/QA/Tracking/StateClusterResidualsQA.cc` around lines 185 - 198, The
code calls geometry->getGlobalPosition(state->get_cluskey(), cluster) before
verifying cluster is non-null, risking a null dereference from
cluster_map->findCluster; change the control flow so you first call auto
*cluster = cluster_map->findCluster(state->get_cluskey()) and immediately check
if (cluster) before invoking geometry->getGlobalPosition or using cluster, and
only then compute glob, cluster_x/cluster_y/cluster_z and fill
m_histograms_x[h], m_histograms_y[h], m_histograms_z[h]; leave state->get_x(),
get_y(), get_z() as needed but ensure getGlobalPosition is not called with a
null cluster.
| StateClusterResidualsQA& setNMvtx(int min, int max) | ||
| { | ||
| m_pending.back().min_mvtx_clusters = min; | ||
| m_pending.back().max_mvtx_clusters = max; | ||
| return *this; | ||
| } | ||
| StateClusterResidualsQA& setNIntt(int min, int max) | ||
| { | ||
| m_pending.back().min_intt_clusters = min; | ||
| m_pending.back().max_intt_clusters = max; | ||
| return *this; | ||
| } | ||
| StateClusterResidualsQA& setNTpc(int min, int max) | ||
| { | ||
| m_pending.back().min_tpc_clusters = min; | ||
| m_pending.back().max_tpc_clusters = max; | ||
| return *this; | ||
| } | ||
| StateClusterResidualsQA& setPhiRange(float min, float max) | ||
| { | ||
| m_pending.back().phi_min = min; | ||
| m_pending.back().phi_max = max; | ||
| return *this; | ||
| } | ||
| StateClusterResidualsQA& setEtaRange(float min, float max) | ||
| { | ||
| m_pending.back().eta_min = min; | ||
| m_pending.back().eta_max = max; | ||
| return *this; | ||
| } | ||
| StateClusterResidualsQA& setPtRange(float min, float max) | ||
| { | ||
| m_pending.back().pt_min = min; | ||
| m_pending.back().pt_max = max; | ||
| return *this; | ||
| } | ||
| StateClusterResidualsQA& setPositiveTracks() | ||
| { | ||
| m_pending.back().charge = 1; | ||
| return *this; | ||
| } | ||
| StateClusterResidualsQA& setNegativeTracks() | ||
| { | ||
| m_pending.back().charge = -1; | ||
| return *this; | ||
| } |
There was a problem hiding this comment.
Calling setters before addHistogram() will cause undefined behavior.
All setter methods access m_pending.back() without checking if the vector is non-empty. If a user calls any setter (e.g., setNMvtx()) before addHistogram(), this triggers undefined behavior on an empty vector.
Consider adding a defensive check or documenting the required call order in the header.
Example defensive check
StateClusterResidualsQA& setNMvtx(int min, int max)
{
+ if (m_pending.empty())
+ {
+ std::cerr << "Error: setNMvtx called before addHistogram()" << std::endl;
+ return *this;
+ }
m_pending.back().min_mvtx_clusters = min;
m_pending.back().max_mvtx_clusters = max;
return *this;
}📝 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.
| StateClusterResidualsQA& setNMvtx(int min, int max) | |
| { | |
| m_pending.back().min_mvtx_clusters = min; | |
| m_pending.back().max_mvtx_clusters = max; | |
| return *this; | |
| } | |
| StateClusterResidualsQA& setNIntt(int min, int max) | |
| { | |
| m_pending.back().min_intt_clusters = min; | |
| m_pending.back().max_intt_clusters = max; | |
| return *this; | |
| } | |
| StateClusterResidualsQA& setNTpc(int min, int max) | |
| { | |
| m_pending.back().min_tpc_clusters = min; | |
| m_pending.back().max_tpc_clusters = max; | |
| return *this; | |
| } | |
| StateClusterResidualsQA& setPhiRange(float min, float max) | |
| { | |
| m_pending.back().phi_min = min; | |
| m_pending.back().phi_max = max; | |
| return *this; | |
| } | |
| StateClusterResidualsQA& setEtaRange(float min, float max) | |
| { | |
| m_pending.back().eta_min = min; | |
| m_pending.back().eta_max = max; | |
| return *this; | |
| } | |
| StateClusterResidualsQA& setPtRange(float min, float max) | |
| { | |
| m_pending.back().pt_min = min; | |
| m_pending.back().pt_max = max; | |
| return *this; | |
| } | |
| StateClusterResidualsQA& setPositiveTracks() | |
| { | |
| m_pending.back().charge = 1; | |
| return *this; | |
| } | |
| StateClusterResidualsQA& setNegativeTracks() | |
| { | |
| m_pending.back().charge = -1; | |
| return *this; | |
| } | |
| StateClusterResidualsQA& setNMvtx(int min, int max) | |
| { | |
| if (m_pending.empty()) | |
| { | |
| std::cerr << "Error: setNMvtx called before addHistogram()" << std::endl; | |
| return *this; | |
| } | |
| m_pending.back().min_mvtx_clusters = min; | |
| m_pending.back().max_mvtx_clusters = max; | |
| return *this; | |
| } | |
| StateClusterResidualsQA& setNIntt(int min, int max) | |
| { | |
| m_pending.back().min_intt_clusters = min; | |
| m_pending.back().max_intt_clusters = max; | |
| return *this; | |
| } | |
| StateClusterResidualsQA& setNTpc(int min, int max) | |
| { | |
| m_pending.back().min_tpc_clusters = min; | |
| m_pending.back().max_tpc_clusters = max; | |
| return *this; | |
| } | |
| StateClusterResidualsQA& setPhiRange(float min, float max) | |
| { | |
| m_pending.back().phi_min = min; | |
| m_pending.back().phi_max = max; | |
| return *this; | |
| } | |
| StateClusterResidualsQA& setEtaRange(float min, float max) | |
| { | |
| m_pending.back().eta_min = min; | |
| m_pending.back().eta_max = max; | |
| return *this; | |
| } | |
| StateClusterResidualsQA& setPtRange(float min, float max) | |
| { | |
| m_pending.back().pt_min = min; | |
| m_pending.back().pt_max = max; | |
| return *this; | |
| } | |
| StateClusterResidualsQA& setPositiveTracks() | |
| { | |
| m_pending.back().charge = 1; | |
| return *this; | |
| } | |
| StateClusterResidualsQA& setNegativeTracks() | |
| { | |
| m_pending.back().charge = -1; | |
| return *this; | |
| } |
🤖 Prompt for AI Agents
In `@offline/QA/Tracking/StateClusterResidualsQA.h` around lines 56 - 101, All
setter methods (setNMvtx, setNIntt, setNTpc, setPhiRange, setEtaRange,
setPtRange, setPositiveTracks, setNegativeTracks) access m_pending.back()
without ensuring m_pending is non-empty; update each setter to guard against an
empty m_pending by either creating a default pending entry (e.g., if
(m_pending.empty()) m_pending.emplace_back();) before using back(), or by
throwing/checking with a clear error message instructing the caller to call
addHistogram() first—apply the same fix to every setter so m_pending.back() is
always safe to access.
| std::vector<TH1*> m_histograms_z{}; | ||
| }; | ||
|
|
||
| #endif // TRACKFITTINGQA_H |
There was a problem hiding this comment.
Incorrect include guard comment.
The #endif comment references TRACKFITTINGQA_H but the actual include guard is STATECLUSTERRESIDUALSQA_H. This appears to be a copy-paste error.
Suggested fix
-#endif // TRACKFITTINGQA_H
+#endif // STATECLUSTERRESIDUALSQA_H📝 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.
| #endif // TRACKFITTINGQA_H | |
| `#endif` // STATECLUSTERRESIDUALSQA_H |
🤖 Prompt for AI Agents
In `@offline/QA/Tracking/StateClusterResidualsQA.h` at line 127, The trailing
`#endif` comment is wrong: replace the incorrect comment "TRACKFITTINGQA_H" with
the actual include guard name "STATECLUSTERRESIDUALSQA_H" so the end of the
header matches the opening guard; update the comment after the `#endif`
accordingly in StateClusterResidualsQA.h to read `#endif` //
STATECLUSTERRESIDUALSQA_H.
13 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)
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.