move OnCal to Fun4Cal#4166
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive calibration reconstruction framework for fun4cal. It defines a base calibration class interface (CalReco) derived from SubsysReco, a feature-rich calibration server (Fun4CalServer) derived from Fun4AllServer with ODBC database integration, enumeration constants for status codes and histogram binning, and autotools build configuration supporting the new library. 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}
| CalReco::CalReco(const std::string &Name) | ||
| : SubsysReco(Name) | ||
| { | ||
| } | ||
|
|
||
| int CalReco::process_event(PHCompositeNode * /*topNode*/) | ||
| { | ||
| std::cout << "process_event(PHCompositeNode *topNode) not implemented by daughter class: " << Name() << std::endl; | ||
| return -1; | ||
| } | ||
|
|
||
| int CalReco::End(PHCompositeNode * /*topNode*/) | ||
| { | ||
| std::cout << "EndOfAnalysis not implemented by subsystem!" << std::endl; | ||
| std::cout << "Use this signal for computing your calibrations and commit." << std::endl; | ||
| std::cout << "Dont do these operations at EndOfRun since subsystems may be feeded events from different runs." << std::endl; | ||
| std::cout << "The number of events is the real parameter here, not the runnumber." << std::endl; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Document analysis impact / reprocessing expectation.
This introduces a calibration reconstruction framework and DB workflow; please update the PR description to state expected analysis impact and whether reprocessing is required.
Based on learnings: If the PR changes reconstruction outputs, calibration constants, or simulation behavior, ensure the description states expected analysis impact and whether reprocessing is required.
| class CalReco : public SubsysReco | ||
| { | ||
| public: | ||
| ~CalReco() override = default; | ||
|
|
||
| // These might be overwritten by everyone... | ||
| int process_event(PHCompositeNode *topNode) override; | ||
| int End(PHCompositeNode *topNode) override = 0; // Here you analyze and commit (if committing flag is set) | ||
|
|
||
| // Thsse control committing to the database... | ||
| virtual void CommitToPdbCal(const int value) = 0; // Set the flag for whether EndOfAnalysis will commit or not | ||
| virtual int VerificationOK() const = 0; // Tell us whether the new calib is close enough to the old one | ||
| virtual int CommitedToPdbCalOK() const = 0; // Tell us whether committing was successful by re-reading the data | ||
|
|
There was a problem hiding this comment.
Clarify analysis impact and downstream updates for the new CalReco interface.
Line 10 onward introduces a new calibration base class and commit/verification contract. Please update the PR description to state whether calibration constants or reconstruction outputs change and whether reprocessing is required, and call out any downstream macros/code that must migrate to Fun4Cal/CalReco.
Based on learnings: If the PR changes reconstruction outputs, calibration constants, or simulation behavior, ensure the description states expected analysis impact and whether reprocessing is required.
As per coding guidelines: If interfaces change, ask for compatibility notes and any needed downstream updates.
| int Fun4CalServer::BeginRun(const int runno) | ||
| { | ||
| if (runno <= 0) | ||
| { | ||
| std::cout << PHWHERE << "Invalid Run Number: " << runno << std::endl; | ||
| exit(1); | ||
| } | ||
| FillRunListFromFileList(); | ||
| recoConsts *rc = recoConsts::instance(); | ||
| // we stick to the first runnumber, but after inheriting from | ||
| // Fun4All we get a EndRun/BeginRun when the run number changes | ||
| // so we have to catch this here | ||
| if (RunNumber() != 0) | ||
| { | ||
| rc->set_IntFlag("RUNNUMBER", RunNumber()); // set rc flag back to previous run | ||
| analysed_runs.push_back(runno); | ||
| return 0; | ||
| } | ||
| RunNumber(runno); | ||
| std::vector<std::pair<SubsysReco *, PHCompositeNode *> >::iterator iter; | ||
| // copy the subsys reco pointers to another set for | ||
| // easier search (we only need the pointers to find | ||
| // the subsystems with special timestamp/runnumber needs | ||
| std::set<SubsysReco *> NeedOtherTimeStamp; | ||
| std::map<std::string, std::set<SubsysReco *> >::const_iterator miter; | ||
| std::set<SubsysReco *>::const_iterator siter; | ||
| for (miter = requiredCalibrators.begin(); | ||
| miter != requiredCalibrators.end(); ++miter) | ||
| { | ||
| for (siter = miter->second.begin(); siter != miter->second.end(); ++siter) | ||
| { | ||
| NeedOtherTimeStamp.insert(*siter); | ||
| } | ||
| } | ||
|
|
||
| int iret; | ||
| int i = 0; | ||
| int oncalrun = runno; | ||
| int fun4allrun = runno; | ||
|
|
||
| RunToTime *runTime = RunToTime::instance(); | ||
| PHTimeStamp *ts = runTime->getBeginTime(fun4allrun); | ||
| PHTimeStamp OnCalBORTimeStamp = *ts; | ||
| PHTimeStamp Fun4AllBORTimeStamp(OnCalBORTimeStamp); | ||
| delete ts; | ||
| if (!requiredCalibrators.empty()) | ||
| { | ||
| fun4allrun = FindClosestCalibratedRun(runno); | ||
| ts = runTime->getBeginTime(fun4allrun); | ||
| Fun4AllBORTimeStamp = *ts; | ||
| delete ts; | ||
| } | ||
|
|
||
| // we have to do the same TDirectory games as in the Init methods | ||
| // save the current dir, cd to the subsystem name dir (which was | ||
| // created in init) call the InitRun of the module and cd back | ||
|
|
||
| gROOT->cd(default_Tdirectory.c_str()); | ||
| std::string currdir = gDirectory->GetPath(); | ||
| std::set<std::string> droplist; | ||
| for (iter = Subsystems.begin(); iter != Subsystems.end(); ++iter) | ||
| { | ||
| std::ostringstream newdirname; | ||
| newdirname << (*iter).second->getName() << "/" << (*iter).first->Name(); | ||
| if (!gROOT->cd(newdirname.str().c_str())) | ||
| { | ||
| std::cout << PHWHERE << "Unexpected TDirectory Problem cd'ing to " | ||
| << (*iter).second->getName() | ||
| << " - send e-mail to off-l with your macro" << std::endl; | ||
| exit(1); | ||
| } | ||
| CalReco *oncal = dynamic_cast<CalReco *>((*iter).first); | ||
| if (oncal) | ||
| { | ||
| std::string table = "CalReco"; | ||
| table += (*iter).first->Name(); | ||
| check_create_subsystable(table); | ||
| insertRunNumInDB(table, runNum); | ||
| std::string calibname = (*iter).first->Name(); | ||
| add_calibrator_to_statustable(calibname); | ||
| std::set<int>::const_iterator runiter; | ||
| int calibstatus = GetCalibStatus(calibname, runNum); | ||
| if (calibstatus > 0 && testmode == 0) | ||
| { | ||
| std::cout << calibname << " already ran for run " << runNum << std::endl; | ||
| droplist.insert(calibname); | ||
| unregisterSubsystem(oncal); | ||
| unregisterHisto(calibname); | ||
| } | ||
| else | ||
| { | ||
| std::ostringstream stringarg; | ||
| stringarg << Fun4CalDBCodes::STARTED; | ||
| for (runiter = runlist.begin(); runiter != runlist.end(); ++runiter) | ||
| { | ||
| updateDB(successTable, calibname, stringarg.str(), *runiter); | ||
| } | ||
| } | ||
| } | ||
| if (NeedOtherTimeStamp.contains((*iter).first)) | ||
| { | ||
| std::cout << "changing timestamp for " << (*iter).first->Name() << std::endl; | ||
| rc->set_IntFlag("RUNNUMBER", fun4allrun); | ||
| // rc->set_TimeStamp(Fun4AllBORTimeStamp); | ||
| } | ||
| else | ||
| { | ||
| rc->set_IntFlag("RUNNUMBER", oncalrun); | ||
| // rc->set_TimeStamp(CalRecoBORTimeStamp); | ||
| } | ||
| if (!droplist.contains((*iter).first->Name())) | ||
| { | ||
| iret = (*iter).first->InitRun(TopNode); | ||
| if (iret == Fun4AllReturnCodes::ABORTRUN) | ||
| { | ||
| std::cout << PHWHERE << "Module " << (*iter).first->Name() << " issued Abort Run, exiting" << std::endl; | ||
| exit(-1); | ||
| } | ||
| i += iret; | ||
| } | ||
| } | ||
| gROOT->cd(currdir.c_str()); | ||
|
|
||
| rc->set_IntFlag("RUNNUMBER", oncalrun); | ||
| // rc->set_TimeStamp(CalRecoBORTimeStamp); | ||
| if (Fun4CalServerVars->GetBinContent(Fun4CalHistoBinDefs::FIRSTRUNBIN) == 0) | ||
| { | ||
| Fun4CalServerVars->SetBinContent(Fun4CalHistoBinDefs::FIRSTRUNBIN, runno); | ||
| Fun4CalServerVars->SetBinContent(Fun4CalHistoBinDefs::BORTIMEBIN, (Stat_t) OnCalBORTimeStamp.getTics()); | ||
| } | ||
| Fun4CalServerVars->SetBinContent(Fun4CalHistoBinDefs::LASTRUNBIN, (Stat_t) runno); | ||
| ts = runTime->getEndTime(runno); | ||
| if (ts) | ||
| { | ||
| Fun4CalServerVars->SetBinContent(Fun4CalHistoBinDefs::EORTIMEBIN, (Stat_t) ts->getTics()); | ||
| delete ts; | ||
| } | ||
|
|
||
| // disconnect from DB to save resources on DB machine | ||
| // PdbCal leaves the DB connection open (PdbCal will reconnect without | ||
| // problem if neccessary) | ||
| DisconnectDB(); | ||
| // finally drop calibrators which have run already from module list | ||
| unregisterSubsystemsNow(); | ||
| return i; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check C++ standard flags in build configuration files
find . -type f \( -name 'Makefile*' -o -name 'configure*' -o -name 'CMakeLists.txt' \) | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1079
🏁 Script executed:
# Search for RunToTime class definition
rg -n 'class RunToTime|RunToTime::getBeginTime' --type cpp --type hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 136
🏁 Script executed:
# Check for any null checks of getBeginTime results in the codebase
rg -n 'getBeginTime' -A 3 --type cpp --type h | head -50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 3661
🏁 Script executed:
# Search for std::set::contains usage in the codebase to gauge C++ version
rg -n '\.contains\(' --type cpp --type h | head -30Repository: sPHENIX-Collaboration/coresoftware
Length of output: 3256
🏁 Script executed:
# Check the configure.ac for fun4cal to see C++ standard
cat calibrations/framework/fun4cal/configure.acRepository: sPHENIX-Collaboration/coresoftware
Length of output: 423
🏁 Script executed:
# Check Makefile.am for C++ flags
cat calibrations/framework/fun4cal/Makefile.amRepository: sPHENIX-Collaboration/coresoftware
Length of output: 926
🏁 Script executed:
# Check root configure.ac for C++ standard
cat configure.ac 2>/dev/null || find . -maxdepth 2 -name 'configure.ac' -exec head -30 {} \;Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Search for any C++ standard flags in the entire configure system
rg -i 'cxx_std|--std=c\+\+|AC_PROG_CXX' configure.ac Makefile.am 2>/dev/null | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check if there's a top-level configuration file
ls -la | grep -E 'configure|CMake|setup'Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Look for README or documentation about build requirements
find . -maxdepth 2 -type f \( -name 'README*' -o -name 'INSTALL*' -o -name '.github' \) 2>/dev/nullRepository: sPHENIX-Collaboration/coresoftware
Length of output: 89
🏁 Script executed:
# Check the actual line numbers in Fun4CalServer.cc around the vulnerable code
sed -n '260,275p' calibrations/framework/fun4cal/Fun4CalServer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 520
🏁 Script executed:
# Check the safer pattern used elsewhere in same file
sed -n '1564,1577p' calibrations/framework/fun4cal/Fun4CalServer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 391
🏁 Script executed:
# Check README for build requirements and C++ standard
head -100 README.mdRepository: sPHENIX-Collaboration/coresoftware
Length of output: 3591
🏁 Script executed:
# Check if there are any CI/workflow files that show build flags
find . -path './.github' -o -path './ci' -o -path './.travis.yml' 2>/dev/null | head -10Repository: sPHENIX-Collaboration/coresoftware
Length of output: 87
🏁 Script executed:
# Look for any compiler flag specification at the top-level
cat Makefile.am 2>/dev/null | head -50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Search for explicit C++ standard requirements in any file
rg -i 'c\+\+17|c\+\+20|c\+\+2a' --type-list cpp 2>/dev/null | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check GitHub workflows for build configuration
find .github -type f -name '*.yml' -o -name '*.yaml' 2>/dev/null | head -10Repository: sPHENIX-Collaboration/coresoftware
Length of output: 137
🏁 Script executed:
# Look for compiler/build info in workflow files
cat .github/workflows/*.yml 2>/dev/null | grep -i 'cxx\|c++\|compiler' | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 162
🏁 Script executed:
# Check what the build environment actually uses
find . -name '.github' -type d -exec ls -la {} \;Repository: sPHENIX-Collaboration/coresoftware
Length of output: 321
🏁 Script executed:
# Search for any C++ standard setting in all configure files
find . -name 'configure.ac' -o -name 'configure' -o -name 'Makefile' -type f | xargs grep -l 'std=c++' 2>/dev/null | head -10Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check if AC_PROG_CXX specifies a version elsewhere
find . -name 'configure.ac' -type f -exec grep -H 'AC_PROG_CXX\|CXXFLAGS' {} \; 2>/dev/null | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1820
🏁 Script executed:
# Look at the actual line 303 and 312 where contains is used in Fun4CalServer.cc
sed -n '300,315p' calibrations/framework/fun4cal/Fun4CalServer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 675
🏁 Script executed:
# Get the full context around the contains usage to see exact line numbers
sed -n '295,320p' calibrations/framework/fun4cal/Fun4CalServer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 987
🏁 Script executed:
# Search for the exact lines with .contains in Fun4CalServer.cc
grep -n "\.contains(" calibrations/framework/fun4cal/Fun4CalServer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 188
🏁 Script executed:
# Get context around those lines
sed -n '300,315p' calibrations/framework/fun4cal/Fun4CalServer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 675
🏁 Script executed:
# Check OnCalServer.cc for the same issues mentioned in review
grep -n "\.contains(" calibrations/framework/oncal/OnCalServer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 188
🏁 Script executed:
# Get the getBeginTime calls in OnCalServer.cc context
sed -n '260,280p' calibrations/framework/oncal/OnCalServer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 762
🏁 Script executed:
# Search for any -std flag or C++ version info in environment or setup scripts
find . -name '*.sh' -o -name '*.bash' -o -name 'setup*' | xargs grep -l 'std=' 2>/dev/null | head -5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check if there's a top-level makefile or build system that might set standards
ls -la | grep -i makeRepository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check if the codebase explicitly requires C++20
grep -r "c++20\|c++2a\|std=2" . --include="*.ac" --include="*.am" --include="CMake*" 2>/dev/null | head -10Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check the git log or any documentation about C++ version requirement
git log --all --oneline -- configure.ac | head -10Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Look for any mention in wiki or docs about compiler/C++ requirement
find . -name '*.md' -type f | xargs grep -i 'c++17\|c++20\|compiler' 2>/dev/null | head -10Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Final verification: check if there are any comments or patterns suggesting C++20 adoption
grep -r "C++20\|C++17\|std::set::contains" . --include="*.h" --include="*.cc" 2>/dev/null | head -15Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
# Check what version of GCC/Clang the project targets
find . -name '.travis.yml' -o -name '*.yml' -o -name '*.yaml' | xargs cat 2>/dev/null | grep -i 'gcc\|clang\|compiler' | head -10Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
Guard RunToTime lookups and avoid C++20-only contains.
RunToTime::getBeginTime can return null; dereferencing it without a guard will crash. The code dereferences ts at lines 264–267 and 271–273 without null checks, while other parts of the same file properly validate the pointer. Additionally, std::set::contains at lines 322 and 333 requires C++20; the build configuration does not explicitly specify a C++ standard, risking compilation failures. Replace contains() with find() and add null checks before pointer dereference.
🔧 Suggested fix
- PHTimeStamp *ts = runTime->getBeginTime(fun4allrun);
- PHTimeStamp OnCalBORTimeStamp = *ts;
- PHTimeStamp Fun4AllBORTimeStamp(OnCalBORTimeStamp);
- delete ts;
+ PHTimeStamp *ts = runTime->getBeginTime(fun4allrun);
+ if (!ts)
+ {
+ std::cout << PHWHERE << "Unknown Run " << fun4allrun << std::endl;
+ exit(1);
+ }
+ PHTimeStamp OnCalBORTimeStamp = *ts;
+ PHTimeStamp Fun4AllBORTimeStamp(OnCalBORTimeStamp);
+ delete ts;
if (!requiredCalibrators.empty())
{
fun4allrun = FindClosestCalibratedRun(runno);
ts = runTime->getBeginTime(fun4allrun);
+ if (!ts)
+ {
+ std::cout << PHWHERE << "Unknown Run " << fun4allrun << std::endl;
+ exit(1);
+ }
Fun4AllBORTimeStamp = *ts;
delete ts;
}
@@
- if (NeedOtherTimeStamp.contains((*iter).first))
+ if (NeedOtherTimeStamp.find((*iter).first) != NeedOtherTimeStamp.end())
{
@@
- if (!droplist.contains((*iter).first->Name()))
+ if (droplist.find((*iter).first->Name()) == droplist.end())
{| int Fun4CalServer::End() | ||
| { | ||
| if (nEvents == 0) | ||
| { | ||
| std::cout << "No Events read, you probably gave me an empty filelist" << std::endl; | ||
| return -1; | ||
| } | ||
| int i = 0; | ||
| std::vector<std::pair<SubsysReco *, PHCompositeNode *> >::iterator iter; | ||
| gROOT->cd(default_Tdirectory.c_str()); | ||
| std::string currdir = gDirectory->GetPath(); | ||
|
|
||
| for (iter = Subsystems.begin(); iter != Subsystems.end(); ++iter) | ||
| { | ||
| std::ostringstream newdirname; | ||
| newdirname << (*iter).second->getName() << "/" << (*iter).first->Name(); | ||
| if (!gROOT->cd(newdirname.str().c_str())) | ||
| { | ||
| std::cout << PHWHERE << "Unexpected TDirectory Problem cd'ing to " | ||
| << (*iter).second->getName() | ||
| << " - send e-mail to off-l with your macro" << std::endl; | ||
| exit(1); | ||
| } | ||
| else | ||
| { | ||
| if (Verbosity() > 2) | ||
| { | ||
| std::cout << "End: cded to " << newdirname.str().c_str() << std::endl; | ||
| } | ||
| } | ||
| i += (*iter).first->End((*iter).second); | ||
| } | ||
|
|
||
| gROOT->cd(default_Tdirectory.c_str()); | ||
| currdir = gDirectory->GetPath(); | ||
| for (iter = Subsystems.begin(); iter != Subsystems.end(); ++iter) | ||
| { | ||
| CalReco *oncal = dynamic_cast<CalReco *>((*iter).first); | ||
| if (!oncal) | ||
| { | ||
| continue; | ||
| } | ||
| std::ostringstream newdirname; | ||
| newdirname << (*iter).second->getName() << "/" << (*iter).first->Name(); | ||
| if (!gROOT->cd(newdirname.str().c_str())) | ||
| { | ||
| std::cout << PHWHERE << "Unexpected TDirectory Problem cd'ing to " | ||
| << (*iter).second->getName() | ||
| << " - send e-mail to off-l with your macro" << std::endl; | ||
| exit(1); | ||
| } | ||
|
|
||
| std::string CalibratorName = oncal->Name(); | ||
|
|
||
| int verificationstatus = oncal->VerificationOK(); | ||
| int databasecommitstatus = oncal->CommitedToPdbCalOK(); | ||
|
|
||
| // report success database the status of the calibration | ||
| if (recordDB) | ||
| { | ||
| std::string table = "CalReco"; | ||
| table += CalibratorName; | ||
|
|
||
| std::ostringstream stringarg; | ||
| if (databasecommitstatus == Fun4CalDBCodes::SUCCESS) | ||
| { | ||
| stringarg << Fun4CalDBCodes::COVERED; | ||
| } | ||
| else | ||
| { | ||
| stringarg << Fun4CalDBCodes::FAILED; | ||
| } | ||
| std::set<int>::const_iterator runiter; | ||
| for (runiter = runlist.begin(); runiter != runlist.end(); ++runiter) | ||
| { | ||
| updateDB(successTable, CalibratorName, stringarg.str(), *runiter); | ||
| } | ||
| // update the first run which was used in the calibration | ||
| // with the real status | ||
| updateDB(successTable, CalibratorName, databasecommitstatus); | ||
|
|
||
| stringarg.str(""); | ||
| stringarg << databasecommitstatus; | ||
| updateDB(table, "committed", stringarg.str(), RunNumber()); | ||
|
|
||
| stringarg.str(""); | ||
| stringarg << verificationstatus; | ||
| updateDB(table, "verified", stringarg.str(), RunNumber()); | ||
|
|
||
| odbc::Timestamp stp(time(nullptr)); | ||
| updateDB(table, "date", stp.toString(), RunNumber()); | ||
| updateDB(table, "comment", oncal->Comment(), RunNumber()); | ||
| time_t beginticks = beginTimeStamp.getTics(); | ||
| stringarg.str(""); | ||
| stringarg << beginticks; | ||
| updateDB(table, "startvaltime", stringarg.str(), RunNumber()); | ||
| stp.setTime(beginticks); | ||
| updateDB(table, "begintime", stp.toString(), RunNumber()); | ||
| time_t endticks = endTimeStamp.getTics(); | ||
| stringarg.str(""); | ||
| stringarg << endticks; | ||
| updateDB(table, "endvaltime", stringarg.str(), RunNumber()); | ||
| stp.setTime(endticks); | ||
| updateDB(table, "endtime", stp.toString(), RunNumber()); | ||
|
|
||
| std::string filelist; | ||
| for (Fun4AllSyncManager *sync : SyncManagers) | ||
| { | ||
| for (Fun4AllInputManager *inmgr : sync->GetInputManagers()) | ||
| { | ||
| for (const std::string &infile : inmgr->GetFileOpenedList()) | ||
| { | ||
| filelist += (infile).substr(((infile).find_last_of('/') + 1), (infile).size()); | ||
| filelist += " "; // this needs to be stripped again for last entry | ||
| } | ||
| } | ||
| } | ||
| filelist.pop_back(); // strip empty space at end from loop | ||
| std::cout << "FileList: " << filelist << std::endl; | ||
| updateDB(table, "files", filelist, RunNumber()); | ||
| updateDB(table, "cvstag", cvstag, RunNumber()); | ||
| } | ||
|
|
||
| std::cout << "SERVER SUMMARY: " << oncal->Name() << " " | ||
| << (verificationstatus == 1 ? "Verification: SUCCESS " : "Verification: FAILURE ") | ||
| << (databasecommitstatus == 1 ? "DB commit: SUCCESS " : "DB commit: FAILURE ") | ||
| << std::endl; | ||
|
|
||
| printStamps(); | ||
| } | ||
| gROOT->cd(currdir.c_str()); | ||
| dumpHistos(); // save the histograms in files | ||
| return i; | ||
| } |
There was a problem hiding this comment.
Avoid pop_back() on an empty file list.
If no input files were opened, filelist is empty and pop_back() will crash at End().
🔧 Suggested fix
- filelist.pop_back(); // strip empty space at end from loop
+ if (!filelist.empty())
+ {
+ filelist.pop_back(); // strip empty space at end from loop
+ }📝 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.
| int Fun4CalServer::End() | |
| { | |
| if (nEvents == 0) | |
| { | |
| std::cout << "No Events read, you probably gave me an empty filelist" << std::endl; | |
| return -1; | |
| } | |
| int i = 0; | |
| std::vector<std::pair<SubsysReco *, PHCompositeNode *> >::iterator iter; | |
| gROOT->cd(default_Tdirectory.c_str()); | |
| std::string currdir = gDirectory->GetPath(); | |
| for (iter = Subsystems.begin(); iter != Subsystems.end(); ++iter) | |
| { | |
| std::ostringstream newdirname; | |
| newdirname << (*iter).second->getName() << "/" << (*iter).first->Name(); | |
| if (!gROOT->cd(newdirname.str().c_str())) | |
| { | |
| std::cout << PHWHERE << "Unexpected TDirectory Problem cd'ing to " | |
| << (*iter).second->getName() | |
| << " - send e-mail to off-l with your macro" << std::endl; | |
| exit(1); | |
| } | |
| else | |
| { | |
| if (Verbosity() > 2) | |
| { | |
| std::cout << "End: cded to " << newdirname.str().c_str() << std::endl; | |
| } | |
| } | |
| i += (*iter).first->End((*iter).second); | |
| } | |
| gROOT->cd(default_Tdirectory.c_str()); | |
| currdir = gDirectory->GetPath(); | |
| for (iter = Subsystems.begin(); iter != Subsystems.end(); ++iter) | |
| { | |
| CalReco *oncal = dynamic_cast<CalReco *>((*iter).first); | |
| if (!oncal) | |
| { | |
| continue; | |
| } | |
| std::ostringstream newdirname; | |
| newdirname << (*iter).second->getName() << "/" << (*iter).first->Name(); | |
| if (!gROOT->cd(newdirname.str().c_str())) | |
| { | |
| std::cout << PHWHERE << "Unexpected TDirectory Problem cd'ing to " | |
| << (*iter).second->getName() | |
| << " - send e-mail to off-l with your macro" << std::endl; | |
| exit(1); | |
| } | |
| std::string CalibratorName = oncal->Name(); | |
| int verificationstatus = oncal->VerificationOK(); | |
| int databasecommitstatus = oncal->CommitedToPdbCalOK(); | |
| // report success database the status of the calibration | |
| if (recordDB) | |
| { | |
| std::string table = "CalReco"; | |
| table += CalibratorName; | |
| std::ostringstream stringarg; | |
| if (databasecommitstatus == Fun4CalDBCodes::SUCCESS) | |
| { | |
| stringarg << Fun4CalDBCodes::COVERED; | |
| } | |
| else | |
| { | |
| stringarg << Fun4CalDBCodes::FAILED; | |
| } | |
| std::set<int>::const_iterator runiter; | |
| for (runiter = runlist.begin(); runiter != runlist.end(); ++runiter) | |
| { | |
| updateDB(successTable, CalibratorName, stringarg.str(), *runiter); | |
| } | |
| // update the first run which was used in the calibration | |
| // with the real status | |
| updateDB(successTable, CalibratorName, databasecommitstatus); | |
| stringarg.str(""); | |
| stringarg << databasecommitstatus; | |
| updateDB(table, "committed", stringarg.str(), RunNumber()); | |
| stringarg.str(""); | |
| stringarg << verificationstatus; | |
| updateDB(table, "verified", stringarg.str(), RunNumber()); | |
| odbc::Timestamp stp(time(nullptr)); | |
| updateDB(table, "date", stp.toString(), RunNumber()); | |
| updateDB(table, "comment", oncal->Comment(), RunNumber()); | |
| time_t beginticks = beginTimeStamp.getTics(); | |
| stringarg.str(""); | |
| stringarg << beginticks; | |
| updateDB(table, "startvaltime", stringarg.str(), RunNumber()); | |
| stp.setTime(beginticks); | |
| updateDB(table, "begintime", stp.toString(), RunNumber()); | |
| time_t endticks = endTimeStamp.getTics(); | |
| stringarg.str(""); | |
| stringarg << endticks; | |
| updateDB(table, "endvaltime", stringarg.str(), RunNumber()); | |
| stp.setTime(endticks); | |
| updateDB(table, "endtime", stp.toString(), RunNumber()); | |
| std::string filelist; | |
| for (Fun4AllSyncManager *sync : SyncManagers) | |
| { | |
| for (Fun4AllInputManager *inmgr : sync->GetInputManagers()) | |
| { | |
| for (const std::string &infile : inmgr->GetFileOpenedList()) | |
| { | |
| filelist += (infile).substr(((infile).find_last_of('/') + 1), (infile).size()); | |
| filelist += " "; // this needs to be stripped again for last entry | |
| } | |
| } | |
| } | |
| filelist.pop_back(); // strip empty space at end from loop | |
| std::cout << "FileList: " << filelist << std::endl; | |
| updateDB(table, "files", filelist, RunNumber()); | |
| updateDB(table, "cvstag", cvstag, RunNumber()); | |
| } | |
| std::cout << "SERVER SUMMARY: " << oncal->Name() << " " | |
| << (verificationstatus == 1 ? "Verification: SUCCESS " : "Verification: FAILURE ") | |
| << (databasecommitstatus == 1 ? "DB commit: SUCCESS " : "DB commit: FAILURE ") | |
| << std::endl; | |
| printStamps(); | |
| } | |
| gROOT->cd(currdir.c_str()); | |
| dumpHistos(); // save the histograms in files | |
| return i; | |
| } | |
| int Fun4CalServer::End() | |
| { | |
| if (nEvents == 0) | |
| { | |
| std::cout << "No Events read, you probably gave me an empty filelist" << std::endl; | |
| return -1; | |
| } | |
| int i = 0; | |
| std::vector<std::pair<SubsysReco *, PHCompositeNode *> >::iterator iter; | |
| gROOT->cd(default_Tdirectory.c_str()); | |
| std::string currdir = gDirectory->GetPath(); | |
| for (iter = Subsystems.begin(); iter != Subsystems.end(); ++iter) | |
| { | |
| std::ostringstream newdirname; | |
| newdirname << (*iter).second->getName() << "/" << (*iter).first->Name(); | |
| if (!gROOT->cd(newdirname.str().c_str())) | |
| { | |
| std::cout << PHWHERE << "Unexpected TDirectory Problem cd'ing to " | |
| << (*iter).second->getName() | |
| << " - send e-mail to off-l with your macro" << std::endl; | |
| exit(1); | |
| } | |
| else | |
| { | |
| if (Verbosity() > 2) | |
| { | |
| std::cout << "End: cded to " << newdirname.str().c_str() << std::endl; | |
| } | |
| } | |
| i += (*iter).first->End((*iter).second); | |
| } | |
| gROOT->cd(default_Tdirectory.c_str()); | |
| currdir = gDirectory->GetPath(); | |
| for (iter = Subsystems.begin(); iter != Subsystems.end(); ++iter) | |
| { | |
| CalReco *oncal = dynamic_cast<CalReco *>((*iter).first); | |
| if (!oncal) | |
| { | |
| continue; | |
| } | |
| std::ostringstream newdirname; | |
| newdirname << (*iter).second->getName() << "/" << (*iter).first->Name(); | |
| if (!gROOT->cd(newdirname.str().c_str())) | |
| { | |
| std::cout << PHWHERE << "Unexpected TDirectory Problem cd'ing to " | |
| << (*iter).second->getName() | |
| << " - send e-mail to off-l with your macro" << std::endl; | |
| exit(1); | |
| } | |
| std::string CalibratorName = oncal->Name(); | |
| int verificationstatus = oncal->VerificationOK(); | |
| int databasecommitstatus = oncal->CommitedToPdbCalOK(); | |
| // report success database the status of the calibration | |
| if (recordDB) | |
| { | |
| std::string table = "CalReco"; | |
| table += CalibratorName; | |
| std::ostringstream stringarg; | |
| if (databasecommitstatus == Fun4CalDBCodes::SUCCESS) | |
| { | |
| stringarg << Fun4CalDBCodes::COVERED; | |
| } | |
| else | |
| { | |
| stringarg << Fun4CalDBCodes::FAILED; | |
| } | |
| std::set<int>::const_iterator runiter; | |
| for (runiter = runlist.begin(); runiter != runlist.end(); ++runiter) | |
| { | |
| updateDB(successTable, CalibratorName, stringarg.str(), *runiter); | |
| } | |
| // update the first run which was used in the calibration | |
| // with the real status | |
| updateDB(successTable, CalibratorName, databasecommitstatus); | |
| stringarg.str(""); | |
| stringarg << databasecommitstatus; | |
| updateDB(table, "committed", stringarg.str(), RunNumber()); | |
| stringarg.str(""); | |
| stringarg << verificationstatus; | |
| updateDB(table, "verified", stringarg.str(), RunNumber()); | |
| odbc::Timestamp stp(time(nullptr)); | |
| updateDB(table, "date", stp.toString(), RunNumber()); | |
| updateDB(table, "comment", oncal->Comment(), RunNumber()); | |
| time_t beginticks = beginTimeStamp.getTics(); | |
| stringarg.str(""); | |
| stringarg << beginticks; | |
| updateDB(table, "startvaltime", stringarg.str(), RunNumber()); | |
| stp.setTime(beginticks); | |
| updateDB(table, "begintime", stp.toString(), RunNumber()); | |
| time_t endticks = endTimeStamp.getTics(); | |
| stringarg.str(""); | |
| stringarg << endticks; | |
| updateDB(table, "endvaltime", stringarg.str(), RunNumber()); | |
| stp.setTime(endticks); | |
| updateDB(table, "endtime", stp.toString(), RunNumber()); | |
| std::string filelist; | |
| for (Fun4AllSyncManager *sync : SyncManagers) | |
| { | |
| for (Fun4AllInputManager *inmgr : sync->GetInputManagers()) | |
| { | |
| for (const std::string &infile : inmgr->GetFileOpenedList()) | |
| { | |
| filelist += (infile).substr(((infile).find_last_of('/') + 1), (infile).size()); | |
| filelist += " "; // this needs to be stripped again for last entry | |
| } | |
| } | |
| } | |
| if (!filelist.empty()) | |
| { | |
| filelist.pop_back(); // strip empty space at end from loop | |
| } | |
| std::cout << "FileList: " << filelist << std::endl; | |
| updateDB(table, "files", filelist, RunNumber()); | |
| updateDB(table, "cvstag", cvstag, RunNumber()); | |
| } | |
| std::cout << "SERVER SUMMARY: " << oncal->Name() << " " | |
| << (verificationstatus == 1 ? "Verification: SUCCESS " : "Verification: FAILURE ") | |
| << (databasecommitstatus == 1 ? "DB commit: SUCCESS " : "DB commit: FAILURE ") | |
| << std::endl; | |
| printStamps(); | |
| } | |
| gROOT->cd(currdir.c_str()); | |
| dumpHistos(); // save the histograms in files | |
| return i; | |
| } |
| bool Fun4CalServer::updateDB(const std::string &table, const std::string &column, | ||
| const std::string &entry, const int runno, const bool append) | ||
| { | ||
| if (!DBconnection) | ||
| { | ||
| connectDB(); | ||
| } | ||
|
|
||
| odbc::Statement *statement = nullptr; | ||
|
|
||
| statement = DBconnection->createStatement(); | ||
|
|
||
| std::string comment; | ||
| std::ostringstream cmd; | ||
| if (append) | ||
| { | ||
| odbc::ResultSet *rs = nullptr; | ||
| std::ostringstream query; | ||
| query << "SELECT * FROM " | ||
| << table | ||
| << " WHERE runnumber = " | ||
| << runno; | ||
|
|
||
| try | ||
| { | ||
| rs = statement->executeQuery(query.str()); | ||
| } | ||
| catch (odbc::SQLException &e) | ||
| { | ||
| std::cout << "in function Fun4CalServer::updateDB() ... "; | ||
| std::cout << "run number " << runno << "not found in DB" << std::endl; | ||
| std::cout << e.getMessage() << std::endl; | ||
| } | ||
|
|
||
| rs->next(); | ||
| try | ||
| { | ||
| comment = rs->getString(column); | ||
| comment += " "; // add empty space between comments | ||
| } | ||
| catch (odbc::SQLException &e) | ||
| { | ||
| std::cout << "in function Fun4CalServer::updateDB() ... " << std::endl; | ||
| std::cout << "nothing to append." << std::endl; | ||
| std::cout << e.getMessage() << std::endl; | ||
| } | ||
| delete rs; | ||
| } | ||
|
|
||
| comment += entry; | ||
| cmd << "UPDATE " | ||
| << table | ||
| << " set " | ||
| << column | ||
| << " = '" | ||
| << comment | ||
| << "' WHERE runnumber = " | ||
| << runno; | ||
|
|
||
| if (Verbosity() == 1) | ||
| { | ||
| std::cout << "in function Fun4CalServer::updateDB() ... "; | ||
| std::cout << "executin SQL statement ... " << std::endl; | ||
| std::cout << cmd.str() << std::endl; | ||
| } | ||
|
|
||
| try | ||
| { | ||
| statement->executeUpdate(cmd.str()); | ||
| } | ||
| catch (odbc::SQLException &e) | ||
| { | ||
| std::cout << e.getMessage() << std::endl; | ||
| return false; | ||
| } | ||
| delete statement; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file and check if it exists
find . -name "Fun4CalServer.cc" -type fRepository: sPHENIX-Collaboration/coresoftware
Length of output: 127
🏁 Script executed:
# If file exists, check the relevant lines (945-1022)
if [ -f "calibrations/framework/fun4cal/Fun4CalServer.cc" ]; then
wc -l calibrations/framework/fun4cal/Fun4CalServer.cc
sed -n '945,1022p' calibrations/framework/fun4cal/Fun4CalServer.cc
fiRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1925
🏁 Script executed:
# Search for other uses of executeQuery in the codebase to understand error handling patterns
rg "executeQuery" -A 10 -B 2 --type cpp --type ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 104
🏁 Script executed:
# Look for how ResultSet is used elsewhere to understand null/error handling
rg "ResultSet" -B 3 -A 5 --type cpp --type cc | head -100Repository: sPHENIX-Collaboration/coresoftware
Length of output: 104
🏁 Script executed:
# Search for executeQuery and ResultSet usage patterns without file type restrictions
rg "executeQuery" -A 10 -B 2 | head -150Repository: sPHENIX-Collaboration/coresoftware
Length of output: 10123
🏁 Script executed:
# Look for how the library is used elsewhere - check imports and includes
rg "odbc::" -B 1 -A 3 | head -100Repository: sPHENIX-Collaboration/coresoftware
Length of output: 6480
🏁 Script executed:
# Check if there are other methods in this same file that show proper error handling
rg -A 15 "createStatement\(\)" calibrations/framework/fun4cal/Fun4CalServer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 9288
Fix null pointer dereference in updateDB() append path.
If the SELECT query fails, rs remains nullptr and is dereferenced unconditionally at rs->next(). The exception handler must either return early or the row access must be guarded with a null check.
Suggested fix
try
{
rs = statement->executeQuery(query.str());
}
catch (odbc::SQLException &e)
{
std::cout << "in function Fun4CalServer::updateDB() ... ";
std::cout << "run number " << runno << "not found in DB" << std::endl;
std::cout << e.getMessage() << std::endl;
+ delete statement;
+ return false;
}
-
- rs->next();
- try
- {
- comment = rs->getString(column);
- comment += " "; // add empty space between comments
- }
- catch (odbc::SQLException &e)
- {
- std::cout << "in function Fun4CalServer::updateDB() ... " << std::endl;
- std::cout << "nothing to append." << std::endl;
- std::cout << e.getMessage() << std::endl;
- }
+ if (rs && rs->next())
+ {
+ try
+ {
+ comment = rs->getString(column);
+ comment += " "; // add empty space between comments
+ }
+ catch (odbc::SQLException &e)
+ {
+ std::cout << "in function Fun4CalServer::updateDB() ... " << std::endl;
+ std::cout << "nothing to append." << std::endl;
+ std::cout << e.getMessage() << std::endl;
+ }
+ }
delete rs;| int Fun4CalServer::CopyTables(const CalReco *calibrator, const int FromRun, const int ToRun, const int commit) | ||
| { | ||
| int iret = calibrator->CopyTables(FromRun, ToRun, commit); | ||
| return iret; | ||
| } | ||
|
|
||
| int Fun4CalServer::CreateCalibration(CalReco *calibrator, const int myrunnumber, const std::string &what, const int commit) | ||
| { | ||
| int iret = -1; | ||
| runNum = myrunnumber; | ||
| SetBorTime(myrunnumber); | ||
| SetEorTime(myrunnumber); | ||
| if (!connectDB()) | ||
| { | ||
| std::cout << "could not connect to " << database << std::endl; | ||
| return -1; | ||
| } | ||
| add_calibrator_to_statustable(calibrator->Name()); | ||
| std::string table = "OnCal"; | ||
| table += calibrator->Name(); | ||
| check_create_subsystable(table); | ||
| odbc::Statement *stmt = DBconnection->createStatement(); | ||
| std::ostringstream cmd; | ||
|
|
||
| cmd << "SELECT runnumber FROM " | ||
| << successTable << " where runnumber = " | ||
| << myrunnumber; | ||
| odbc::ResultSet *rs = nullptr; | ||
| try | ||
| { | ||
| rs = stmt->executeQuery(cmd.str()); | ||
| } | ||
| catch (odbc::SQLException &e) | ||
| { | ||
| std::cout << "Table " << successTable << " does not exist" << std::endl; | ||
| return -1; | ||
| } | ||
| if (!rs->next()) | ||
| { | ||
| insertRunNumInDB(successTable, myrunnumber); | ||
| } | ||
| delete rs; | ||
| cmd.str(""); | ||
| cmd << "SELECT runnumber FROM " | ||
| << successTable << " where runnumber = " | ||
| << myrunnumber << " and " | ||
| << calibrator->Name() << " <= 0"; | ||
| try | ||
| { | ||
| rs = stmt->executeQuery(cmd.str()); | ||
| } | ||
| catch (odbc::SQLException &e) | ||
| { | ||
| std::cout << PHWHERE << " Exception caught, Message: " | ||
| << e.getMessage() << std::endl; | ||
| return -1; | ||
| } | ||
| if (rs->next() || testmode) | ||
| { | ||
| std::string tablecomment = "Subsytem provided"; | ||
| iret = calibrator->CreateCalibration(runnumber, what, tablecomment, commit); | ||
| if (!iret) | ||
| { | ||
| std::cout << "Comment: " << tablecomment << std::endl; | ||
| std::cout << "updating oncal status tables for " << runnumber << std::endl; | ||
| if (commit) | ||
| { | ||
| CreateCalibrationUpdateStatus(calibrator, table, tablecomment, Fun4CalDBCodes::SUBSYSTEM); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| std::cout << "Calibratior " << calibrator->Name() << " for run " << runnumber << " failed" << std::endl; | ||
| if (commit) | ||
| { | ||
| CreateCalibrationUpdateStatus(calibrator, table, tablecomment, Fun4CalDBCodes::FAILED); | ||
| } | ||
| } | ||
| } | ||
| else | ||
| { | ||
| std::cout << PHWHERE << " Run " << runnumber << " is already successfully calibrated for " | ||
| << calibrator->Name() << std::endl; | ||
| } | ||
| return iret; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n '\brunnumber\b' calibrations/framework/fun4cal/Fun4CalServer.h calibrations/framework/fun4cal/Fun4CalServer.cc | head -50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 5306
🏁 Script executed:
rg -n '\brunNum\b' calibrations/framework/fun4cal/Fun4CalServer.h calibrations/framework/fun4cal/Fun4CalServer.cc | head -50Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1670
🏁 Script executed:
# Check the class definition to see member variables
sed -n '1,200p' calibrations/framework/fun4cal/Fun4CalServer.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 6070
Fix undefined runnumber identifier—use runNum instead.
runnumber is not declared in this scope and will cause a compilation error. The member variable runNum (initialized at line 1648) is the correct identifier to use, consistent with how the class tracks the current run throughout all other methods.
Changes needed
- iret = calibrator->CreateCalibration(runnumber, what, tablecomment, commit);
+ iret = calibrator->CreateCalibration(runNum, what, tablecomment, commit);
@@
- std::cout << "updating oncal status tables for " << runnumber << std::endl;
+ std::cout << "updating oncal status tables for " << runNum << std::endl;
@@
- std::cout << "Calibratior " << calibrator->Name() << " for run " << runnumber << " failed" << std::endl;
+ std::cout << "Calibratior " << calibrator->Name() << " for run " << runNum << " failed" << std::endl;
@@
- std::cout << PHWHERE << " Run " << runnumber << " is already successfully calibrated for "
+ std::cout << PHWHERE << " Run " << runNum << " is already successfully calibrated for "| using Fun4AllServer::registerHisto; | ||
| void registerHisto(TH1 *h1d, CalReco *Calibrator, const int replace = 0); | ||
| void unregisterHisto(const std::string &calibratorname); | ||
| void Print(const std::string &what = "ALL") const override; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "Fun4CalServer.h" | head -5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 126
🏁 Script executed:
fd "Fun4CalServer.h" --type fRepository: sPHENIX-Collaboration/coresoftware
Length of output: 124
🌐 Web query:
ROOT TH1 histogram ownership rules when storing in containers or servers
💡 Result:
Key ROOT ownership rules for TH1 (esp. when putting them in containers / exposing via servers)
- Default: histograms are owned by the current directory (
gDirectory)
- When you
new TH1..., ROOT automatically adds it togDirectory(often the last openedTFile), and that directory will delete it when the directory/file is closed/destructed. [1][2] - To prevent that for a specific histogram:
h->SetDirectory(nullptr);(then you must delete it). [1][2]
- To prevent it globally for subsequently-created histograms:
TH1::AddDirectory(kFALSE);(then you own them all). [1][3]
- “Putting a
TH1*in a C++ container does not change ownership
std::vector<TH1*>,std::map<...>, etc. just store pointers; they don’t delete unless you do.- Best practice: store histograms as
std::unique_ptr<TH1>and ensure they are not owned by aTDirectory(useTH1::AddDirectory(false)orh->SetDirectory(nullptr)). [1]
- ROOT collections (
TList,TObjArray,TFolder, …) may delete contents only if configured to
- Many ROOT collections have an “owner” mode; e.g.
TFolder::SetOwner(true)makes the folder delete objects added to it when the folder is deleted. [4][5] - If you enable owner mode, make sure the histogram is not also owned by a
TDirectory, otherwise you risk double-deletion (directory deletes it, and the collection also tries to). [1]
- Copy/clone doesn’t keep directory association
TH1::SetDirectory()is not treated as a real persisted property of the histogram for copy/clone; afterClone()you mustSetDirectory(...)again if you want the clone attached to a directory. [2]
- Servers (e.g.
THttpServer) typically browse objects; they don’t “own” them
THttpServer/TRootSniffercan scan the “global” directory content (which includes auto-registered histograms), and you can disable that global scan if you only want explicitly registered items. [6]- Practical implication: if you don’t want intermediate histograms showing up, avoid auto-registration (use
TH1::AddDirectory(false)/SetDirectory(nullptr)), and register only what you want.
Sources: ROOT object ownership manual/user guide and TH1/TFolder reference docs. [1][2][4][5][6]
🏁 Script executed:
cat -n ./calibrations/framework/fun4cal/Fun4CalServer.h | head -60Repository: sPHENIX-Collaboration/coresoftware
Length of output: 2160
🏁 Script executed:
cat -n ./calibrations/framework/fun4cal/Fun4CalServer.h | sed -n '120,140p'Repository: sPHENIX-Collaboration/coresoftware
Length of output: 985
🏁 Script executed:
grep -n "~Fun4CalServer\|destructor" ./calibrations/framework/fun4cal/Fun4CalServer.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 109
🏁 Script executed:
find . -type f -name "Fun4CalServer.cpp" | head -1Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
fd "Fun4CalServer.cc" --type fRepository: sPHENIX-Collaboration/coresoftware
Length of output: 125
🏁 Script executed:
cat -n ./calibrations/framework/fun4cal/Fun4CalServer.cc | grep -A 20 "~Fun4CalServer"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 694
🏁 Script executed:
wc -l ./calibrations/framework/fun4cal/Fun4CalServer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 132
🏁 Script executed:
grep -n "void Fun4CalServer::registerHisto" ./calibrations/framework/fun4cal/Fun4CalServer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 165
🏁 Script executed:
sed -n '1100,1200p' ./calibrations/framework/fun4cal/Fun4CalServer.cc | cat -nRepository: sPHENIX-Collaboration/coresoftware
Length of output: 3718
🏁 Script executed:
sed -n '162,220p' ./calibrations/framework/fun4cal/Fun4CalServer.cc | cat -nRepository: sPHENIX-Collaboration/coresoftware
Length of output: 2107
🏁 Script executed:
grep -n "void Fun4CalServer::unregisterHisto" ./calibrations/framework/fun4cal/Fun4CalServer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 152
🏁 Script executed:
find . -type f -name "Fun4AllServer.h" -o -name "Fun4AllServer.cc" | grep -E "\.h$|\.cc$"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 166
🏁 Script executed:
grep -n "void.*registerHisto" calibrations/framework/fun4cal/Fun4CalServer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 240
🏁 Script executed:
grep -n "void.*registerHisto\|class Fun4AllServer" ./offline/framework/fun4all/Fun4AllServer.h | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 121
🏁 Script executed:
grep -n "std::map.*TH1" ./offline/framework/fun4all/Fun4AllServer.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
grep -A 30 "Fun4AllServer::registerHisto" ./offline/framework/fun4all/Fun4AllServer.cc | head -40Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1472
🏁 Script executed:
grep -B 5 -A 15 "ServerHistoManager" ./offline/framework/fun4all/Fun4AllServer.cc | head -40Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1501
🏁 Script executed:
grep -n "Histo\[" ./calibrations/framework/fun4cal/Fun4CalServer.cc | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
grep -B 5 -A 10 "Fun4CalServerVars" ./calibrations/framework/fun4cal/Fun4CalServer.cc | head -40Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1115
🏁 Script executed:
grep -n "SetDirectory\|AddDirectory" ./calibrations/framework/fun4cal/Fun4CalServer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
grep -n "class Fun4AllHistoManager\|void dumpHistos" ./offline/framework/fun4all/Fun4AllServer.h | head -5Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
grep -B 5 -A 15 "~Fun4AllServer\|~Fun4AllHistoManager" ./offline/framework/fun4all/Fun4AllServer.cc | head -40Repository: sPHENIX-Collaboration/coresoftware
Length of output: 500
🏁 Script executed:
sed -n '/^Fun4AllServer::~Fun4AllServer()/,/^}/p' ./offline/framework/fun4all/Fun4AllServer.cc | head -80Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1478
🏁 Script executed:
grep -n "delete.*Histo\|Histo.*delete" ./calibrations/framework/fun4cal/Fun4CalServer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 60
Document and fix unclear TH1/CalReco/SubsysReco* ownership semantics.**
Fun4CalServer stores raw pointers in Histo map (line 123) and creates Fun4CalServerVars (line 122) via new TH1D(...) but never deletes them in the destructor (lines 82–86). The destructor only cleans up DBconnection, leaving histogram lifetime management unclear:
- Does Fun4CalServer own registered histograms and must clean them up?
- Or do callers retain ownership and must keep them alive until unregistering?
This ambiguity creates risk of memory leaks (if the server should own them) or double-deletion (if ROOT's gDirectory also owns them by default). Currently, ownership is neither stated in the header nor enforced via smart pointers.
Recommended actions:
- Document the ownership contract in the class header.
- If Fun4CalServer owns histograms: delete them in the destructor (or use
std::unique_ptr<TH1>). - If callers own them: use
SetDirectory(nullptr)to remove ROOT's default ownership, and document that callers must manage lifetime.
| PHTimeStamp *GetEndValidityTS(); | ||
|
|
||
| PHTimeStamp *GetBeginValidityTS(); | ||
| void printStamps(); | ||
| PHTimeStamp *GetLastGoodRunTS(CalReco *calibrator, const int irun); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "Fun4CalServer.h" -type fRepository: sPHENIX-Collaboration/coresoftware
Length of output: 126
🏁 Script executed:
head -100 calibrations/framework/fun4cal/Fun4CalServer.h | cat -nRepository: sPHENIX-Collaboration/coresoftware
Length of output: 4762
🏁 Script executed:
sed -n '35,55p' calibrations/framework/fun4cal/Fun4CalServer.h | cat -nRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1031
🏁 Script executed:
grep -n "GetEndValidityTS\|GetBeginValidityTS\|GetLastGoodRunTS" calibrations/framework/fun4cal/Fun4CalServer.hRepository: sPHENIX-Collaboration/coresoftware
Length of output: 228
🏁 Script executed:
grep -r "GetEndValidityTS\|GetBeginValidityTS\|GetLastGoodRunTS" --include="*.cpp" --include="*.h" calibrations/Repository: sPHENIX-Collaboration/coresoftware
Length of output: 629
🏁 Script executed:
sed -n '1,200p' calibrations/framework/fun4cal/Fun4CalServer.cpp | cat -n | head -80Repository: sPHENIX-Collaboration/coresoftware
Length of output: 169
🏁 Script executed:
find . -name "*Fun4CalServer*.cpp" -type fRepository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
find . -name "*.cpp" -path "*/fun4cal/*" -type fRepository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
ls -la calibrations/framework/fun4cal/Repository: sPHENIX-Collaboration/coresoftware
Length of output: 771
🏁 Script executed:
grep -n "PHTimeStamp\|GetEndValidityTS\|GetBeginValidityTS\|GetLastGoodRunTS" calibrations/framework/fun4cal/Fun4CalServer.h | head -30Repository: sPHENIX-Collaboration/coresoftware
Length of output: 515
🏁 Script executed:
sed -n '100,200p' calibrations/framework/fun4cal/Fun4CalServer.h | cat -nRepository: sPHENIX-Collaboration/coresoftware
Length of output: 2440
🏁 Script executed:
sed -n '1,100p' calibrations/framework/fun4cal/Fun4CalServer.cc | cat -nRepository: sPHENIX-Collaboration/coresoftware
Length of output: 3460
🏁 Script executed:
grep -A 5 "GetEndValidityTS\|GetBeginValidityTS\|GetLastGoodRunTS" calibrations/framework/fun4cal/Fun4CalServer.cc | head -40Repository: sPHENIX-Collaboration/coresoftware
Length of output: 569
🏁 Script executed:
wc -l calibrations/framework/fun4cal/Fun4CalServer.ccRepository: sPHENIX-Collaboration/coresoftware
Length of output: 130
🏁 Script executed:
grep -r "GetEndValidityTS\|GetBeginValidityTS\|GetLastGoodRunTS" --include="*.cc" --include="*.C" --include="*.cpp" --include="*.h" | grep -v "^Binary" | head -20Repository: sPHENIX-Collaboration/coresoftware
Length of output: 1201
🏁 Script executed:
sed -n '100,150p' calibrations/framework/fun4cal/Fun4CalServer.cc | cat -nRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1976
🏁 Script executed:
sed -n '100,140p' calibrations/framework/fun4cal/Fun4CalServer.cc | grep -A 15 "GetBeginValidityTS"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 478
🏁 Script executed:
grep -r "GetEndValidityTS\(\)\|GetBeginValidityTS\(\)\|GetLastGoodRunTS" calibrations/ --include="*.cc" --include="*.C" --include="*.cpp" -B 2 -A 3 | head -60Repository: sPHENIX-Collaboration/coresoftware
Length of output: 2500
🏁 Script executed:
grep -n "delete.*GetEndValidityTS\|delete.*GetBeginValidityTS\|delete.*GetLastGoodRunTS" calibrations/ -r --include="*.cc" --include="*.C" --include="*.cpp"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
grep -r "GetEndValidityTS\|GetBeginValidityTS\|GetLastGoodRunTS" --include="*.cc" --include="*.C" --include="*.cpp" --include="*.h" | grep -v "^calibrations/framework/fun4cal/Fun4CalServer" | grep -v "^calibrations/framework/oncal/OnCalServer"Repository: sPHENIX-Collaboration/coresoftware
Length of output: 60
🏁 Script executed:
sed -n '89,115p' calibrations/framework/fun4cal/Fun4CalServer.cc | cat -nRepository: sPHENIX-Collaboration/coresoftware
Length of output: 929
🏁 Script executed:
sed -n '120,170p' calibrations/framework/fun4cal/Fun4CalServer.cc | cat -nRepository: sPHENIX-Collaboration/coresoftware
Length of output: 1981
Clarify ownership semantics for allocated timestamps.
Lines 42-47 return newly allocated PHTimeStamp pointers (via new in the implementation), but the ownership transfer to the caller is undocumented. This creates a memory management contract violation—callers must delete these pointers, but there is no documentation or API marker (e.g., comment, unique_ptr) to communicate this responsibility. Additionally, the methods should be marked const since they do not modify the server's state.
🔧 Suggested API clarification
- PHTimeStamp *GetEndValidityTS();
-
- PHTimeStamp *GetBeginValidityTS();
+ PHTimeStamp *GetEndValidityTS() const;
+
+ PHTimeStamp *GetBeginValidityTS() const;
void printStamps();
- PHTimeStamp *GetLastGoodRunTS(CalReco *calibrator, const int irun);
+ PHTimeStamp *GetLastGoodRunTS(CalReco *calibrator, const int irun) const;Add documentation comment (e.g., in header or implementation) clarifying that the caller owns the returned pointer and must delete it, or consider returning by value instead of new-allocated pointer.
📝 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.
| PHTimeStamp *GetEndValidityTS(); | |
| PHTimeStamp *GetBeginValidityTS(); | |
| void printStamps(); | |
| PHTimeStamp *GetLastGoodRunTS(CalReco *calibrator, const int irun); | |
| PHTimeStamp *GetEndValidityTS() const; | |
| PHTimeStamp *GetBeginValidityTS() const; | |
| void printStamps(); | |
| PHTimeStamp *GetLastGoodRunTS(CalReco *calibrator, const int irun) const; |
Build & test reportReport for commit 062f669b7a372955653d0c982d4c9e045c72eed7:
Automatically generated by sPHENIX Jenkins continuous integration |
1 participant
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
This PR moves OnCal to Fun4Cal, renaming the sources appropriately
TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
Summary: Move OnCal to Fun4Cal
Motivation & Context:
This PR refactors the calibration framework by introducing a new Fun4Cal subsystem, which replaces/extends the OnCal framework. This represents a structured reorganization of calibration infrastructure to provide improved modularity and database integration for the sPHENIX experiment.
Key Changes:
CalReco base class (
CalReco.h/.cc): Introduces an abstract base class derived fromSubsysRecothat serves as the interface for all calibration reconstruction modules. Provides lifecycle hooks (process_event(),End()), database commit/verification methods (CommitToPdbCal(),VerificationOK()), and state management (comment tracking, calibration table management).Fun4CalServer class (
Fun4CalServer.h/.cc): Implements a specialized calibration server derived fromFun4AllServer(~2400 lines) with extensive functionality:Supporting infrastructure:
Fun4CalDBCodes.h: Database status codes (INIT, STARTED, FAILED, SUCCESS, COPIED variants, COVERED, SUBSYSTEM)Fun4CalHistoBinDefs.h: Histogram bin definitions for run and time rangesMakefile.am,configure.ac,autogen.shfor Autotools integrationPotential Risk Areas:
-lodbc++) andpdbcalBaselibraries—verify availability and version compatibility across all deployment targets.Future Improvements:
Note: AI-generated summaries can contain inaccuracies. Cross-reference database interactions and timestamp logic with actual implementation details before deployment.