Skip to content

Make operators inline friends to improve template matching#2347

Merged
sethrj merged 11 commits intoceleritas-project:developfrom
sethrj:friend-comparators
Apr 2, 2026
Merged

Make operators inline friends to improve template matching#2347
sethrj merged 11 commits intoceleritas-project:developfrom
sethrj:friend-comparators

Conversation

@sethrj
Copy link
Copy Markdown
Member

@sethrj sethrj commented Apr 1, 2026

It turns out that many of the operators for Quantity, OpaqueId, etc. have a subtle but unfortunate oversight that, because they're declared as template functions, they don't participate in overload resolution when implicitly casting: this prevents std::ref(value) + 2 from compiling. The fix is to declare them as inline friend functions, where they are reachable by ADL and where the RefWrapper can be automatically looked up via its implicit operator.

Moving the rest of the comparison operators into "inline" massively improves output from failed comparisons (e.g., if you try to test equality for a class that isn't convertible or doesn't have an equal sign). Such functions no longer are output in error messages from unrelated classes.

This change also improves the behavior of allowed comparison with OpaqueId: now MatId{x} < 3 is forbidden; only MatId{x} < y.size() (since the RHS is an unsigned int) works.

@sethrj sethrj requested a review from pcanal April 1, 2026 15:45
@sethrj sethrj requested a review from elliottbiondo as a code owner April 1, 2026 15:45
@sethrj sethrj added core Software engineering infrastructure (corecel) minor Refactoring or minor internal changes/fixes labels Apr 1, 2026
@sethrj sethrj requested review from Copilot and removed request for elliottbiondo April 1, 2026 15:45
Copy link
Copy Markdown

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 adjusts a number of “strong type” wrappers (e.g., Quantity, OpaqueId, Constant, Array, etc.) to define operators as inline friend functions so they participate in ADL and behave better with implicit conversions (notably std::reference_wrapper), and updates affected call sites/tests accordingly.

Changes:

  • Move comparison/arithmetic operators for several core types to inline friend definitions to improve overload resolution/ADL and reduce unrelated template noise in diagnostics.
  • Tighten/adjust some comparator behavior (e.g., unsigned-only integer comparisons for OpaqueId) and update tests/usages to match.
  • Update documentation tooling (Doxygen predefined macros) to reflect new macro shorthands.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
test/corecel/random/Selector.test.cc Update OpaqueId comparisons to avoid signed-integer RHS.
test/corecel/OpaqueId.test.cc Update integer literal to unsigned to match new OpaqueId comparator constraints.
test/corecel/math/Quantity.test.cc Add regression test for std::ref(quantity) operator behavior.
test/celeritas/random/ElementSelector.test.cc Update ElementId comparisons to use typed RHS IDs.
src/orange/orangeinp/detail/InfixStringBuilder.hh Adjust sentinel comparison for surface IDs vs logic tokens.
src/orange/orangeinp/detail/BuildLogic.cc Adjust sentinel comparison for surface IDs vs logic tokens.
src/geocel/BoundingBox.hh Move bbox equality/inequality operators into inline friend definitions.
src/corecel/OpaqueId.hh Redefine OpaqueId comparators/operators as inline friends; restrict integer comparisons.
src/corecel/math/Quantity.hh Add type constraints and move quantity operators/comparators to inline friends.
src/corecel/math/detail/QuantityImpl.hh Add inline friend comparators for UnitlessQuantity.
src/corecel/math/Constant.hh Move constant operators/comparators to inline friend definitions; use new macro shorthands.
src/corecel/io/Label.hh Move label comparators into inline friend definitions.
src/corecel/data/PinnedAllocator.hh Move allocator equality/inequality into inline friend definitions.
src/corecel/data/ObserverPtr.hh Constrain converting ctor; move comparators into inline friend definitions.
src/corecel/data/detail/PinnedAllocatorImpl.cc Remove now-unneeded include after header include changes.
src/corecel/cont/Array.hh Move array equality/inequality into inline friend definitions.
src/celeritas/phys/PDGNumber.hh Move PDGNumber comparators into inline friend definitions.
src/celeritas/phys/AtomicNumber.hh Move AtomicNumber comparators into inline friend definitions.
doc/CMakeLists.txt Update Doxygen predefined macro list for new macro aliases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 1, 2026

Codecov Report

❌ Patch coverage is 96.39640% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.19%. Comparing base (008ef57) to head (0451ecd).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/corecel/OpaqueId.hh 94.59% 0 Missing and 2 partials ⚠️
src/corecel/math/Constant.hh 86.66% 0 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2347      +/-   ##
===========================================
+ Coverage    87.18%   87.19%   +0.01%     
===========================================
  Files         1374     1374              
  Lines        43678    43682       +4     
  Branches     13840    13836       -4     
===========================================
+ Hits         38082    38090       +8     
+ Misses        4538     4536       -2     
+ Partials      1058     1056       -2     
Files with missing lines Coverage Δ
src/celeritas/phys/AtomicNumber.hh 59.09% <100.00%> (ø)
src/celeritas/phys/PDGNumber.hh 96.55% <100.00%> (ø)
src/corecel/cont/Array.hh 100.00% <100.00%> (ø)
src/corecel/data/ObserverPtr.hh 96.42% <100.00%> (ø)
src/corecel/data/PinnedAllocator.hh 100.00% <ø> (ø)
src/corecel/data/detail/PinnedAllocatorImpl.cc 44.00% <ø> (ø)
src/corecel/io/Label.hh 100.00% <100.00%> (ø)
src/corecel/math/Quantity.hh 100.00% <100.00%> (ø)
src/corecel/math/detail/QuantityImpl.hh 83.33% <100.00%> (+3.33%) ⬆️
src/geocel/BoundingBox.hh 98.24% <100.00%> (ø)
... and 9 more

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 1, 2026

Test summary

 5 617 files   9 101 suites   19m 21s ⏱️
 2 247 tests  2 206 ✅  41 💤 0 ❌
31 766 runs  31 646 ✅ 120 💤 0 ❌

Results for commit 0451ecd.

♻️ This comment has been updated with latest results.

Copy link
Copy Markdown
Contributor

@pcanal pcanal left a comment

Choose a reason for hiding this comment

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

LGTM.

@sethrj sethrj force-pushed the friend-comparators branch from 5441091 to 4ede50b Compare April 1, 2026 19:46
@sethrj sethrj merged commit ef6495b into celeritas-project:develop Apr 2, 2026
76 of 78 checks passed
@sethrj sethrj deleted the friend-comparators branch April 2, 2026 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Software engineering infrastructure (corecel) minor Refactoring or minor internal changes/fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants