Skip to content

Conversation

@PetrilloAtWork
Copy link
Member

The class caf::SRTrkChi2PID is not supposed to be different than before, but some coding guidelines have been adopted as promised in another pull request.

  • Removed empty virtual destructor (CF-151).
  • Data member initialization on declaration (CF-155); default constructor removed because unecessary.
  • Replaced std::numeric_limits with caf::kSignalingNaN in value construction.
  • Reduced block nesting (namespace block removed when it's easy to do).
  • Turned the class into a struct, since it's a public data structure and there is no invariant to protect.
  • Updated comments for Doxygen.

A potential breaking change: moving from class to struct may irk the compiler (Clang used to complain, GCC did not) when the class is mentioned in a forward declaration in an inconsistent way (like namespace 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:

  • @JosiePaton (still an official SBN CAF maintainer I think :-) )
  • @kjplows (release manager, because of the GitHub commit business)
  • GitHub Copilot (for the laugh)

Complying to SBN CAF coding guidelines.
Copy link

Copilot AI left a 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 class to struct and removed empty destructor per coding guidelines
  • Implemented member initialization at declaration, removing the need for a custom constructor
  • Replaced std::numeric_limits with caf::kSignalingNaN constant
  • 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>
@kjplows
Copy link
Contributor

kjplows commented Nov 21, 2025

trigger build LArSoft/lar*@LARSOFT_SUITE_v10_12_02

@kjplows kjplows moved this to Testing in SBN software development Nov 21, 2025
@kjplows
Copy link
Contributor

kjplows commented Nov 21, 2025

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 caf::SRTrkChi2PIDProxy is used in at least one place in sbnana which won't get picked up by the sbncode CI.

So I won't merge this just yet, I'd like some time to test this if possible 🙂
Thanks @PetrilloAtWork !

@FNALbuild
Copy link

✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard

@FNALbuild
Copy link

✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard

@FNALbuild
Copy link

❌ 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

@FNALbuild
Copy link

❌ 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

@FNALbuild
Copy link

❌ 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

@FNALbuild
Copy link

⚠️ CI build for SBND Warning at phase ci_tests SBND on slf7 for e26:prof - ignored warnings for build -- details 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

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

Labels

None yet

Projects

Status: Partially reviewed

Development

Successfully merging this pull request may close these issues.

5 participants