Skip to content

Fix CrvDQM_module for updated artdaq-core-mu2e.#7

Merged
eflumerf merged 1 commit intodevelopfrom
eflumerf/FixCrvDQMModule
Feb 12, 2025
Merged

Fix CrvDQM_module for updated artdaq-core-mu2e.#7
eflumerf merged 1 commit intodevelopfrom
eflumerf/FixCrvDQMModule

Conversation

@eflumerf
Copy link
Contributor

No description provided.

@eflumerf eflumerf requested a review from ehrlich-uva February 12, 2025 20:44
@eflumerf eflumerf self-assigned this Feb 12, 2025
@eflumerf
Copy link
Contributor Author

Note that the file has been clang-format'ed, probably a good idea to turn whitespace diff off

Copy link

@ehrlich-uva ehrlich-uva left a comment

Choose a reason for hiding this comment

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

I assume the change in GetCRVHits was the only change.

Comment on lines +291 to +298
auto res = crvData.GetCRVHits(bl, hits);
if(!res)
{
TLOG(TLVL_ERROR) << "Unable to get CRV hist!";
// Iterate invalid count?
// ++invalidEventCounter_;
continue;
}

Choose a reason for hiding this comment

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

I assume that this was the only change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct, the only real change is the GetCRVHits

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why this was needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://clang.llvm.org/extra/clang-tidy/checks/bugprone/unused-return-value.html
The GetCRVHits method was changed to return a boolean indicating whether it was successful. The TLVL_ERROR is there to indicate that the method did not return successfully, and the commented lines were copied from lower in the module, presumably for future error accounting.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this change was picked up when I fixed the merge conflicts earlier, but I'll put it back in.

This module is a bit unfinished, I'm still writing it. Please bear with me.

@eflumerf eflumerf merged commit a6aef20 into develop Feb 12, 2025
2 of 7 checks passed
@eflumerf eflumerf deleted the eflumerf/FixCrvDQMModule branch February 12, 2025 21:32
@eflumerf eflumerf restored the eflumerf/FixCrvDQMModule branch February 12, 2025 21:37
@eflumerf eflumerf deleted the eflumerf/FixCrvDQMModule branch February 12, 2025 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants