Skip to content

Removed exception for invalid offlineID; Added functions to print sipm type name#1805

Open
giro94 wants to merge 2 commits intoMu2e:mainfrom
giro94:dev
Open

Removed exception for invalid offlineID; Added functions to print sipm type name#1805
giro94 wants to merge 2 commits intoMu2e:mainfrom
giro94:dev

Conversation

@giro94
Copy link
Copy Markdown
Collaborator

@giro94 giro94 commented Apr 22, 2026

No description provided.

@giro94 giro94 requested a review from rlcee April 22, 2026 15:45
@FNALbuild
Copy link
Copy Markdown
Collaborator

Hi @giro94,
You have proposed changes to files in these packages:

  • CaloConditions
  • DataProducts

which require these tests: build.

@Mu2e/fnalbuild-users, @Mu2e/write have access to CI actions on main.

⌛ The following tests have been triggered for 96d3cbb: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

@FNALbuild
Copy link
Copy Markdown
Collaborator

☀️ The build tests passed at 96d3cbb.

Test Result Details
test with Command did not list any other PRs to include
merge Merged 96d3cbb at 314d115
build (prof) Log file. Build time: 04 min 10 sec
ceSimReco Log file.
g4test_03MT Log file.
transportOnly Log file.
POT Log file.
g4study Log file.
cosmicSimReco Log file.
cosmicOffSpill Log file.
ceSteps Log file.
ceDigi Log file.
muDauSteps Log file.
ceMix Log file.
rootOverlaps Log file.
g4surfaceCheck Log file.
trigger Log file.
check_cmake Log file.
FIXME, TODO TODO (0) FIXME (0) in 3 files
clang-tidy ➡️ 1 errors 14 warnings
whitespace check no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 96d3cbb after being merged into the base branch at 314d115.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

Copy link
Copy Markdown
Collaborator

@rlcee rlcee left a comment

Choose a reason for hiding this comment

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

It looks like you didn't add a mf call, so please remove the include, thanks

@oksuzian
Copy link
Copy Markdown
Collaborator

AI review — findings from an automated static pass. Please sanity-check each item against intent before acting.

SHA reviewed: 96d3cbbc425a3c504761073f007fc62c4de689d2.

Bugs / Semantic risks

1. Silent-invalid return is now the caller's problem — needs a callsite audit.
The old CaloDAQMap::offlineId() threw twice: once on invalid input, once on invalid lookup result. This PR removes only the second throw:
```cpp
// new
return _raw2Offline[rawId.id()]; // may be an invalid CaloSiPMId
```
Every caller of offlineId() must now check isValid() on the return. The PR body is empty, so it's not clear which callers were audited. Until that's confirmed (e.g. grep -rn 'offlineId(' --include='*.cc'), this is a latent source of invalid IDs flowing into downstream code that previously assumed the map always returned valid. Either list the audited callsites in the PR description, or add a short // caller must check isValid() comment to the header declaration.

2. Header uses std::ostream but doesn't include any ostream header.
DataProducts/inc/CaloConst.hh adds:
```cpp
std::ostream& operator<<(std::ostream& ost, const CaloConst::detType& type);
```
but the only new include is <string>. <string> does not pull in <iosfwd> on all libstdc++ / libc++ versions. Any TU that includes CaloConst.hh without having already included an ostream header will fail to compile. Fix: add #include <iosfwd> (cheapest) or #include <ostream> in the header.

3. Dead include.
CaloDAQMap.cc adds #include \"messagefacility/MessageLogger/MessageLogger.h\" but the remaining code uses no mf:: / LOG_ symbols. Drop it.

Code quality

4. static const std::string detTypeName(detType type) — the top-level const is meaningless on a by-value return and will trigger -Wignored-qualifiers on gcc. Change to static std::string detTypeName(detType type). Even better: static std::string_view detTypeName(detType type) returning string-literal views — avoids a per-call heap allocation in the operator<< path.

5. Style reformat bundled with the behavior change.
Most of the diff (both CaloDAQMap.cc and CaloConst.hh) is pure re-indentation / brace-style. Fine per se, but it buries the one real semantic change (#1) inside a large "looks like noise" diff. Easier to review as a separate commit.

6. default: branch in detTypeName is unreachable.
The detType enum has exactly 4 values (CsI, CAPHRI, PINDiode, Invalid) and all are cased. The default: return \"UNKNOWN\"; never fires under defined behavior. Either remove it (and let -Wswitch catch future additions), or keep it with a comment that it exists to tolerate out-of-range integer casts.

Nits

7. Pre-existing typo next door: _nPINDiodPerDisk (missing the "e") sits next to _nPINDiodeLaserBox. Not this PR's fault, but since the file is already being touched, a rename to _nPINDiodePerDisk would tidy it up — leave if you want the PR to stay tight.

8. Switch case order in detTypeName (CsI, PINDiode, CAPHRI, Invalid) differs from the enum declaration order (CsI, CAPHRI, PINDiode, Invalid). Cosmetic; aligning them helps readability.

@giro94
Copy link
Copy Markdown
Collaborator Author

giro94 commented Apr 23, 2026

It looks like you didn't add a mf call, so please remove the include, thanks

that slipped, thanks

@giro94
Copy link
Copy Markdown
Collaborator Author

giro94 commented Apr 23, 2026

@FNALbuild build test

@giro94
Copy link
Copy Markdown
Collaborator Author

giro94 commented Apr 23, 2026

@FNALbuild build

@FNALbuild
Copy link
Copy Markdown
Collaborator

⌛ The following tests have been triggered for 6efd5e1: build (Build queue - API unavailable)

@FNALbuild
Copy link
Copy Markdown
Collaborator

☀️ The build tests passed at 6efd5e1.

Test Result Details
test with Command did not list any other PRs to include
merge Merged 6efd5e1 at 314d115
build (prof) Log file. Build time: 04 min 12 sec
ceSimReco Log file.
g4test_03MT Log file.
transportOnly Log file.
POT Log file.
g4study Log file.
cosmicSimReco Log file.
cosmicOffSpill Log file.
ceSteps Log file.
ceDigi Log file.
muDauSteps Log file.
ceMix Log file.
rootOverlaps Log file.
g4surfaceCheck Log file.
trigger Log file.
check_cmake Log file.
FIXME, TODO TODO (0) FIXME (0) in 3 files
clang-tidy ➡️ 1 errors 14 warnings
whitespace check no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 6efd5e1 after being merged into the base branch at 314d115.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants