o2-eve, o2-eve-export-workflow, o2-eve-convert: serialisation to root…#9497
Conversation
|
there is no trailing spaces in O2DPLDisplay.cxx |
|
fullCI failed on irrelevant problems |
shahor02
left a comment
There was a problem hiding this comment.
Hi,
Please see comments below. Also in plenty of places you are passing string by copy, please use const std::string& instead.
If you prefer, I can merge it as is now and you address questions later.
|
|
||
| public: | ||
| FileWatcher(const std::string& path); | ||
| FileWatcher(const std::string& path, std::vector<std::string> ext); |
There was a problem hiding this comment.
why passing the vector by copy?
There was a problem hiding this comment.
It is executed once in a program lifetime (may be executed again on source switch, but seldom done) so I prefer code readibility over efficiency - passing by reference will require creating variable just over place where it is called. If you request I can refactor it in next PR (so please merge it as it is now with request to refactor)
There was a problem hiding this comment.
Not sure what you mean, now you are creating a copy to pass a vector, by passing the reference you create nothing.
There was a problem hiding this comment.
And such variable will be visible in outer space - when it has no usage.
something like:
static std::vectorstd::string extensions = ...
There was a problem hiding this comment.
there is some misunderstanding: as it is now you are creating 2 clones of the vector: 1st one just to pass the vector, 2nd as a data member of the class.
To get rid of the 1st copy you just need to define your constructor as:
FileWatcher(const std::string& path, const std::vector<std::string>& ext) : mExt(ext) ... {}
To get rid of the 2nd one: define your class as
class FileWatcher
{
....
const std::vector<std::string>& mExt;
};
| calo_energy = calorimeter.getEnergy(); | ||
| calo_eta = calorimeter.getEta(); | ||
| calo_phi = calorimeter.getPhi(); | ||
| calo_GID = calorimeter.getGIDAsString(); |
There was a problem hiding this comment.
what is the reason of using string for GID, which is a simple integer?
There was a problem hiding this comment.
It was preserved from the previous version to keep interoperability with previous *.json files. I will address that in next (separate to that) PR when I will be able to test this change only. Please merge as it is and I will fix it soon.
shahor02
left a comment
There was a problem hiding this comment.
Merging but please get rid of excessive copies I mentioned.
shahor02
left a comment
There was a problem hiding this comment.
there are actually conflicts, please rebase to dev and resubmit.
Please consider the following formatting changes to AliceO2Group#9497
shahor02
left a comment
There was a problem hiding this comment.
Tests were passed, merging after rebase.
… files, conversion tool