TRD: Move Digit class to DataFormatsTRD#6014
Conversation
martenole
left a comment
martenole
left a comment
There was a problem hiding this comment.
Thanks Jorge that looks good. Would just consider the minor changes in the helper class methods
| static int getMCMfromPad(int irow, int icol) | ||
| { | ||
| if (irow < 0 || icol < 0 || irow > constants::NROWC1 || icol > constants::NCOLUMN) { | ||
| return -1; |
There was a problem hiding this comment.
I know this is written simply analog to what was there in AliRoot, but now that I see it again and think about it shouldn't we put an error message here in case of a wrong input? Because where these functions are used the return values are not checked for plausibility.
There was a problem hiding this comment.
These methods can be called a lot. Since the return value is a flag for errors, I don't think one should add messages here, even if it's just a DEBUG message. So, I would leave this as it is or add an assertion for the whole test condition.
There was a problem hiding this comment.
Especially if the function is called a lot, we should probably not force the user to check for errors. I think this should be a fatal error: if we do not get irow and icol correct, then there is a major issue somewhere else in the code.
There was a problem hiding this comment.
My point here is that this should never happen. These methods are primarily internal, and if there is an error, it should pop up in a test (less likely, be caught eventually as a bug when you start seeing -1 around). Therefore it will require a change in the codebase. So, we expect that this actually never happens or rarely happens. That's why we should use assert, instead of logging, IMO.
We had a similar discussion with @sawenzel a long time ago (See: this code review), and the choice was using assert.
Since this condition should never happen or at least is very unlikely, one could use some optimizations, like: https://en.cppreference.com/w/cpp/language/attributes/likely, but I am not sure how this applies to O2. Of course, this is not relevant if one drops the conditional here.
|
Ah |
|
|
||
| } // namespace trd | ||
| } // namespace o2 | ||
| #include "DataFormatsTRD/Digit.h" |
There was a problem hiding this comment.
Is this a temporary hack, or is this meant to stay? I think we should avoid having files that are essentially unused.
There was a problem hiding this comment.
This would be temporary in order to not break the QC build which looks for the Digit header in TRDBase. After this is merged and QC is updated the file can be deleted
* Move Digit to DataFormats * Use correct header in macro * TRD: back compatibility for QC
4 participants
For moving the Digit clas to DataFormatsTRD, HelperMethods is added in DataFormatsTRD. Otherwise, a cyclic dependency appears between TRDBase and DataFormatTRD.