FCS pi0 finder for Ecal by Xilin#250
Conversation
…gle, IndentWidth: 3, ColumnLimit: 0}" -i
veprbl
left a comment
veprbl
left a comment
There was a problem hiding this comment.
I took a liberty to reformat the source (feel free to revert). Don't see any logical issues, left some comments about technical ones.
| mFcsDb->setDbAccess(0); | ||
| if (!mFcsDb) { | ||
| LOG_ERROR << "StFcsEventDisplay::InitRun Failed to get StFcsDbMaker" << endm; | ||
| return kStFatal; | ||
| } |
There was a problem hiding this comment.
Makes no sense to first dereference a pointer and then check it for NULL.
| Int_t StFcsPi0FinderForEcal::Finish() { | ||
| if (filename.length() == 0) return kStOk; | ||
| const char* fn = filename.c_str(); | ||
| TFile* MyFile = new TFile(fn, "RECREATE"); |
There was a problem hiding this comment.
This leaks memory. Consider switching to
| TFile* MyFile = new TFile(fn, "RECREATE"); | |
| TFile MyFile(fn, "RECREATE"); |
or using std::unique_ptr<TFile>
There was a problem hiding this comment.
Yes. But it is in Finish() not in Make(). Still good idea.
| float E_min = 1; | ||
|
|
||
| #ifndef SKIPDefImp | ||
| ClassDef(StFcsPi0FinderForEcal, 0) |
There was a problem hiding this comment.
Not sure why this is behind an ifdef, our https://github.com/star-bnl/star-sw/blob/main/mgr/RootCint.pl will still see this and try to include it in a generated LinkDef.
There was a problem hiding this comment.
This is what Gene did as a trick to include this maker fastoffline run off from STARDEV while this Maker is not in DEV (StSpinPool). He said this trick works.
|
@veprbl raised very reasonable concerns about the code. Too bad they were ignored... |
|
I merged this PR to get QA moving. I don't disagree that @veprbl 's points are reasonable and the authors can & should address them promptly (soon), but I don't perceive those points as more critical than QA'ing data we are taking at the experiment right_now (this PR already took a week). Given @veprbl 's approval of this PR, perhaps he felt the same. |
|
@akioogawa New PR is the way to make further changes. Thanks! |
|
See PR #271 |
5 participants
This will be needed for FCS pi0 peaks to be part of FastOffline.