-
Notifications
You must be signed in to change notification settings - Fork 14
Standardization of caf::SRTrkChi2PID form #175
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
base: develop
Are you sure you want to change the base?
Conversation
Following CAF coding guidelines.
Complying to SBN CAF coding guidelines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR standardizes the caf::SRTrkChi2PID class to follow established SBN coding conventions. The changes modernize the code structure while maintaining functional equivalence.
Key changes:
- Converted from
classtostructand removed empty destructor per coding guidelines - Implemented member initialization at declaration, removing the need for a custom constructor
- Replaced
std::numeric_limitswithcaf::kSignalingNaNconstant - Updated documentation to Doxygen format
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| SRTrkChi2PID.h | Modernized struct definition with member initialization, updated to Doxygen comments, and converted from class to struct |
| SRTrkChi2PID.cxx | Simplified implementation by removing constructor/destructor, retained setDefault() method with namespace block removal |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Found by Copilot. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
trigger build LArSoft/lar*@LARSOFT_SUITE_v10_12_02 |
|
I'm running the CI, this change looks innocuous but 1) I'm unaware how that might play with (flat)CAF creation and 2) I see that So I won't merge this just yet, I'd like some time to test this if possible 🙂 |
|
✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard |
|
✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard |
|
❌ CI build for SBND Failed at phase build SBND on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build SBND phase logs parent CI build details are available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard 🚨 For more details about the failed phase, check the build ICARUS phase logs parent CI build details are available through the CI dashboard |
|
❌ CI build for ICARUS Failed at phase ci_tests ICARUS on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard 🚨 For more details about the failed phase, check the ci_tests ICARUS phase logs parent CI build details are available through the CI dashboard |
|
🚨 For more details about the warning phase, check the ci_tests SBND phase logs parent CI build details are available through the CI dashboard |
The class
caf::SRTrkChi2PIDis not supposed to be different than before, but some coding guidelines have been adopted as promised in another pull request.CF-151).CF-155); default constructor removed because unecessary.std::numeric_limitswithcaf::kSignalingNaNin value construction.classinto astruct, since it's a public data structure and there is no invariant to protect.A potential breaking change: moving from
classtostructmay irk the compiler (Clang used to complain, GCC did not) when the class is mentioned in a forward declaration in an inconsistent way (likenamespace caf { class SRTrkChi2PID; }). This use is quite rare and trivial to correct.Also, I am trying a GitHub commit for this simple change, meaning that, against all good practices, the code has not been compiled. If this is acceptable, I rely on the automatic build to check the correctness of the new syntax.
Reviewers: