Standardization of caf::SRTrkChi2PID form#175
Conversation
Following CAF coding guidelines.
Complying to SBN CAF coding guidelines.
There was a problem hiding this comment.
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 |
|
trigger build LArSoft/lar*@LARSOFT_SUITE_v10_14_02 SBNSoftware/sbndcode@v10_14_02 SBNSoftware/sbn*@SBN_SUITE_v10_14_02 |
|
✔️ 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 |
|
🚨 For more details about the warning phase, check the ci_tests SBND 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 |
|
With my apologies for taking so long to address this, all looks good. Thanks Gianluca! |
|
trigger build LArSoft/lar*@LARSOFT_SUITE_v10_14_02_02 SBNSoftware/sbndcode@v10_14_02_03 SBNSoftware/sbn*@SBN_SUITE_v10_14_02_03 |
|
✔️ 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 ICARUS Failed at phase build ICARUS on slf7 for e26: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 SBND Failed at phase build SBND on slf7 for e26: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 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 |
|
trigger build LArSoft/lar*@LARSOFT_SUITE_v10_14_02_02 SBNSoftware/sbndaq-artdaq-core@v1_10_06 SBNSoftware/sbndcode@v10_14_02_03 SBNSoftware/sbn*@SBN_SUITE_v10_14_02_03 |
|
✔️ 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 |
|
🚨 For more details about the warning phase, check the ci_tests SBND phase logs parent CI build details are available through the CI dashboard |
|
🚨 For more details about the warning phase, check the ci_tests ICARUS phase logs parent CI build details are available through the CI dashboard |
github-project-automation
bot
moved this from To merge
to Done
in SBN software development
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: