Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid code duplication in SiPixelRawToDigi ErrorChecker's #26731

Closed
perrotta opened this issue May 9, 2019 · 5 comments
Closed

Avoid code duplication in SiPixelRawToDigi ErrorChecker's #26731

perrotta opened this issue May 9, 2019 · 5 comments

Comments

@perrotta
Copy link
Contributor

perrotta commented May 9, 2019

In PR #26679 the ErrorChecker class was differentiated for Phase0 and Phase1.

In it, from the previous single ErrorChecker two classes

  • ErrorChecker (for Phase1)
  • ErrorCheckerPhase0

were defined, both deriving from a common base class ErrorCheckerBase.

In the implementation of that PR, basically all methods in the base class are virtual, and the specializations are implemented separately for the two derived classes.

However, almost all those methods are identical in the two implementations, and they were just copy-pasted from one class to the other. The only exceptions are the two methods:

  • checkROC
  • errorDetId

Moreover, errorDetId is basically identical in the two derived classes: it could also be moved into the base class if one specializes a function which returns (DetId(detIdx.rawId).subdetId() == static_cast<int>(PixelSubdetector::PixelBarrel)) as a bool in Phase0, and just true in Phase1

In order to avoid a lot of (error prone during maintenance) copy/paste one should move all common methods in the base class, and only specialize in the derived ones the two different ones. In case some specialization in future will be required for the Phase1 code (as the Phase0 one is expected to remain frozen for the time being, excluding the adaptations that will be needed in case of some modification up-hill or in the framework), one could simply specialize that method in the derived class once again.

This should get fixed for the next cycle after the one for the UL release: it will be 11_0

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2019

A new Issue was created by @perrotta .

@davidlange6, @Dr15Jones, @smuzaffar, @fabiocos, @kpedro88 can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@perrotta
Copy link
Contributor Author

perrotta commented May 9, 2019

assign reconstruction

@cmsbuild
Copy link
Contributor

cmsbuild commented May 9, 2019

New categories assigned: reconstruction

@slava77,@perrotta you have been requested to review this Pull request/Issue and eventually sign? Thanks

@slava77
Copy link
Contributor

slava77 commented Jul 22, 2021

+reconstruction

via #33884

@cmsbuild
Copy link
Contributor

This issue is fully signed and ready to be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants