Clear the three most concentrated clang-tidy checks - #272
Merged
Conversation
google-build-explicit-make-pair (32 findings, mbo/strings/numbers.h):
a real defect. `std::make_pair<uint32_t, unsigned>(...)` with explicit
template arguments pins the parameters to `uint32_t&&`/`unsigned&&`,
which defeats the deduction make_pair exists for. Use std::pair directly,
as the diagnostic says.
performance-unnecessary-value-param (25 findings, mbo/container/any_scan.h):
one source location, re-reported per template instantiation. Note the
motivation is NOT copy cost -- MakeAnyScanData is 16 bytes holding one
shared_ptr, so a copy is a single atomic increment, and by-value vs
const& compile to the identical indirect parameter (both `ptr` in IR;
the type is not register-transportable because shared_ptr is not
trivially copyable). By-value simply buys nothing here: the const
shared_ptr member means even a "move" copies. const& drops one atomic
pair per construction.
readability-implicit-bool-conversion (23 findings): not defects.
* diff_myers.cc: `(d + lo) & 1` is an idiomatic parity test. Allow
integer and pointer conditions via check options, which is standard
C++ style and clears these legitimately.
* hash_differential_test.cc: inside the XXH64 / XXH3_* reference
macros the check misattributes a conversion to the gtest `<<`
message chain -- it points at string literals like "len: " and
proposes replacing them with `true`. Suppressed with a rationale,
extending the NOLINT region the file already uses.
All three checks now report zero. Tests pass across the touched
packages (9/9).
Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com>
helly25
enabled auto-merge (squash)
August 8, 2026 17:12
Fab-Cat
approved these changes
Aug 8, 2026
helly25
added a commit
that referenced
this pull request
Aug 9, 2026
#272 changed AnyScanImpl's constructors to take MakeAnyScanData by const reference. That silently broke the three public wrappers - AnyScan, ConstScan and ConvertingScan - which take the same type BY VALUE and then `std::move(data)` into AnyScanImpl. Moving into a const& parameter is a no-op, so those moves became misleading dead code and the by-value parameters became "copied but only used as a const reference". The finding count went from 25 (at the impl constructor) to 45 (at the three wrappers): my verification for #272 confirmed the specific diagnostic cleared without re-measuring the file, so the regression went unnoticed. Make the three wrappers take a const reference too and drop the pointless `std::move`. That is consistent with the type: 16 bytes holding one const shared_ptr, whose const member makes even a move a copy. performance-unnecessary-value-param now reports zero for the header. bazel test --config=clang //... - 109/109 pass. Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com>
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
2 participants
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.
Second of the clang-tidy triage PRs, independent of #271 (different files). Clears 80 of the 639 remaining findings.
Only one of the three turned out to be an actual defect. That is worth stating plainly, because "80 findings fixed" would otherwise overstate it.
google-build-explicit-make-pair— 32 findings, a real defectmbo/strings/numbers.husedstd::make_pair<uint32_t, unsigned>(...). Explicit template arguments pin the parameters touint32_t&&/unsigned&&, defeating the deductionmake_pairexists to provide. Nowstd::pairdirectly, as the diagnostic recommends.performance-unnecessary-value-param— 25 findings, marginalAll 25 are one source location in
mbo/container/any_scan.h, re-reported per template instantiation.The motivation is not copy cost, and I checked rather than assumed:
MakeAnyScanDatais 16 bytes — a singleshared_ptr. A copy is one atomic increment.const&compile to an identical parameter: bothptrin the IR. The type is not register-transportable, becauseshared_ptris not trivially copyable. ([[clang::trivial_abi]]does force[2 x ptr], but clang simultaneously warns the attribute "cannot be applied" — an ABI hazard, so not an option.)const shared_ptrmember means even a "move" copies, verified: afterData b{std::move(a)}the source still holds its pointer anduse_count == 2.So by-value buys nothing here;
const&drops one atomic pair per construction. Real but tiny — this is tidiness, not performance. Happy to drop this hunk if you would rather not churn the signature.readability-implicit-bool-conversion— 23 findings, not defectsdiff_myers.cc(2):(d + lo) & 1is an idiomatic parity test. EnabledAllowIntegerConditions/AllowPointerConditions, which is standard C++ style (if (ptr),if (n)) and clears these legitimately.hash_differential_test.cc(21): a false positive. Inside theXXH64/XXH3_*reference macros the check misattributes a conversion to the gtest<<message chain — it points at the string literal"len: "and proposes replacing it withtrue. Suppressed with that rationale, extending theNOLINTregion the file already uses.Test
bazel test --config=clang //mbo/strings:all //mbo/container:any_scan_test //mbo/diff:all //mbo/hash:hash_differential_test— 9/9 pass.pre-commit run -agreen.