Skip to content

Conversation

@lynnt20
Copy link
Contributor

@lynnt20 lynnt20 commented Dec 10, 2025

@kjplows
Copy link
Contributor

kjplows commented Dec 14, 2025

@lynnt20 could you nominate reviewers please?

@kjplows kjplows moved this to Open pull requests in SBN software development Dec 14, 2025
@linyan-w linyan-w moved this to Expected for Later in SBND 2025 Fall Production Dec 15, 2025
Copy link
Member

@PetrilloAtWork PetrilloAtWork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class version business must be fixed.
I have left also a technical C++ comment: fell completely free to decide how to approach it.

@lynnt20
Copy link
Contributor Author

lynnt20 commented Jan 14, 2026

Hi @PetrilloAtWork, thank you for all your comments and suggestions! I believe I've implemented all the changes now: I've added the class versions and checksums for all the new classes, and also removed the value constructor (go back to "plain old data" class).

Copy link
Member

@PetrilloAtWork PetrilloAtWork left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left two completely optional comments, but the PR is approved.

*/

struct LightCalo {
public:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now redundant (struct members are public by default).

public:

// NaN value to initialize data members
static constexpr double nan = std::numeric_limits<double>::signaling_NaN();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow the capitalisation NaN is so "standard" that even C++, that uses all small letters, uses it. Consider replacing nan with NaN.

@kjplows kjplows moved this from Open pull requests to Testing in SBN software development Jan 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Testing
Status: Expected for Later

Development

Successfully merging this pull request may close these issues.

4 participants