Tuple: Silence modernize-pass-by-value - #5646
Merged
WeiqunZhang merged 1 commit intoAug 26, 2026
Merged
Conversation
clang-tidy's modernize-pass-by-value wants this constructor to take its
argument by value and move it. Doing so would be a pessimization here, and
the reason is not obvious from the code, so record it.
The forwarding constructor right below already takes everything that can be
deduced, lvalues and rvalues of T alike:
P p{1,2};
E<P> a(p); // forwarding constructor
E<P> b(P{3,4}); // forwarding constructor
E<P> c({5,6}); // this one -- U cannot be deduced from a braced list
So the const-reference overload is reached only by arguments the forwarding
constructor cannot deduce, a braced initializer list above all. Taking it by
value would add a move for exactly those callers and save a copy for nobody.
This surfaces in the SIMD CI job rather than the others because clang-tidy
runs on ccache misses, and a change to a widely included header is what makes
Tuple.H one of them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ax3l
commented
Aug 25, 2026
Comment on lines
+32
to
+37
| // This overload is what catches arguments the forwarding constructor below | ||
| // cannot deduce, a braced initializer list above all. Everything else, both | ||
| // lvalues and rvalues of T, already goes there. Taking this one by value | ||
| // would therefore add a move for exactly those callers while saving a copy | ||
| // for nobody. | ||
| // NOLINTNEXTLINE(modernize-pass-by-value) |
Member
Author
There was a problem hiding this comment.
Open to either compact or explained version:
Suggested change
| // This overload is what catches arguments the forwarding constructor below | |
| // cannot deduce, a braced initializer list above all. Everything else, both | |
| // lvalues and rvalues of T, already goes there. Taking this one by value | |
| // would therefore add a move for exactly those callers while saving a copy | |
| // for nobody. | |
| // NOLINTNEXTLINE(modernize-pass-by-value) | |
| // NOLINTNEXTLINE(modernize-pass-by-value) |
modernize-pass-by-value
WeiqunZhang
approved these changes
Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
clang-tidy'smodernize-pass-by-valuefires ondetail::gpu_tuple_element's constructor and asks for the argument to be taken by value and moved. Following that advice would be a pessimization here. This adds aNOLINTNEXTLINEwith the reasoning written down, because it is not obvious from the code.Why by value is wrong here
The forwarding constructor immediately below already takes everything that can be deduced — lvalues and rvalues of
Talike. The const-reference overload is reached only by arguments it cannot deduce, a braced initializer list above all:(verified by instrumenting the two constructors and running the three cases)
So taking it by value would add a move for exactly the callers this overload exists to serve, and save a copy for nobody, since rvalues never reach it. Removing the overload instead is not an option either — the braced-init case would stop compiling.
Why it is showing up now
Nothing changed in
AMReX_Tuple.H. The SIMD CI job runsclang-tidyover ccache misses, so which headers get linted depends on what was recompiled. A change to a widely included header — #5644 touchesAMReX_Math.H— pullsAMReX_Tuple.Hinto the linted set and the finding appears. It is pre-existing ondevelopmentand unrelated to that work, which is why it is split out here: this is a two-line comment change that can be reviewed on its own, and #5644 can rebase on it afterwards.Testing
over
Tests/SIMD/main.cppandTests/Particles/ParticleReduceSIMD/main.cpp:modernize-pass-by-valuefindings across all AMReX headers go from 1 to 0, and this was the only one.AMReX_Tuple.Hstill compiles and behaves the same (amrex::makeTuplewith scalar and aggregate members).🤖 Generated with Claude Code