From ed2c013fae030ad5253e2f59408840f0132534f9 Mon Sep 17 00:00:00 2001 From: helly25 <6420169+helly25@users.noreply.github.com> Date: Sat, 8 Aug 2026 21:25:21 +0100 Subject: [PATCH] Clear readability-identifier-length by renaming, not by configuring The plan had been to widen `IgnoredParameterNames` with `m`, on the understanding that it was the gtest matcher idiom. It is not: in mbo/testing/matchers.h the `m` is a MAP - `AllKeys(const Map& m)`, `IsKeyOf(const Map& m)` - and in matchers_test.cc `m` and `mm` are a map and a multimap. Renaming them to `map` and `multi_map` is both clearer and shorter than teaching the linter to ignore a letter repo-wide, so .clang-tidy is untouched. The rest, case by case: * diff_benchmark.cc - `to` -> `to_pos`, `j` -> `pos`. * hash_benchmark.cc - `d` -> `dist_idx`, `b` -> `point`. Not `dist`: that name is already taken in the same loop by `const LatencyDist& dist`, and the obvious rename would have produced `dist = kLatencyDists[dist]`. * ini_file.{h,cc} - `_` -> `unused` for the ignored half of a structured binding. * diff_myers.cc - waived for the file. `n`/`m` are the sequence lengths, `d` the edit cost, `k` the diagonal, `x`/`y` the grid coordinates, `kf`/`kr` the forward and reverse diagonals, `lo`/`hi` the window bounds. That is the notation of Myers' paper, and expanding it would break the correspondence that makes the code checkable. Verified the waiver is narrow: 20 findings from other checks still fire in diff_myers.cc. The check reports zero across all affected files, and bazel test --config=clang //... passes 109/109. Signed-off-by: helly25 <6420169+helly25@users.noreply.github.com> --- mbo/diff/diff_benchmark.cc | 8 ++--- mbo/diff/impl/diff_myers.cc | 10 ++++++ mbo/file/ini/ini_file.cc | 2 +- mbo/file/ini/ini_file.h | 2 +- mbo/hash/hash_benchmark.cc | 12 +++---- mbo/testing/matchers.h | 28 +++++++-------- mbo/testing/matchers_test.cc | 70 ++++++++++++++++++------------------ 7 files changed, 71 insertions(+), 61 deletions(-) diff --git a/mbo/diff/diff_benchmark.cc b/mbo/diff/diff_benchmark.cc index 05dd1df0..be20cdac 100644 --- a/mbo/diff/diff_benchmark.cc +++ b/mbo/diff/diff_benchmark.cc @@ -90,7 +90,7 @@ std::string LongLines( return text; } -std::string MovedBlock(std::size_t count, std::string_view tag, std::size_t from, std::size_t len, std::size_t to) { +std::string MovedBlock(std::size_t count, std::string_view tag, std::size_t from, std::size_t len, std::size_t to_pos) { const std::string base = NumberedLines(count, tag); std::vector lines = absl::StrSplit(base, '\n'); lines.pop_back(); @@ -101,9 +101,9 @@ std::string MovedBlock(std::size_t count, std::string_view tag, std::size_t from continue; } result.push_back(lines[i]); - if (i == to) { - for (std::size_t j = from; j < from + len; ++j) { - result.push_back(lines[j]); + if (i == to_pos) { + for (std::size_t pos = from; pos < from + len; ++pos) { + result.push_back(lines[pos]); } } } diff --git a/mbo/diff/impl/diff_myers.cc b/mbo/diff/impl/diff_myers.cc index 188312c9..4f81a0f6 100644 --- a/mbo/diff/impl/diff_myers.cc +++ b/mbo/diff/impl/diff_myers.cc @@ -32,6 +32,14 @@ #include "mbo/file/artefact.h" #include "mbo/hash/hash.h" +// This file uses the notation from Myers' paper throughout: `n`/`m` are the two +// sequence lengths, `d` the edit cost, `k` the diagonal, `x`/`y` the grid +// coordinates, `kf`/`kr` the forward and reverse diagonals, and `lo`/`hi` the +// window bounds. Expanding those to three-character names would break the +// correspondence to the paper that makes this code checkable, so the length +// rule is waived here rather than per line. +// NOLINTBEGIN(readability-identifier-length) + namespace mbo::diff { namespace { @@ -310,3 +318,5 @@ DiffMyers::Snake DiffMyers::FindMiddleSnake(const Span& span) { } } // namespace mbo::diff + +// NOLINTEND(readability-identifier-length) diff --git a/mbo/file/ini/ini_file.cc b/mbo/file/ini/ini_file.cc index 1a85e4cf..5983d4e9 100644 --- a/mbo/file/ini/ini_file.cc +++ b/mbo/file/ini/ini_file.cc @@ -89,7 +89,7 @@ absl::Status IniFile::Write(std::string_view filename) const { std::size_t IniFile::size() const { std::size_t size = 0; - for (const auto& [_, kvs] : data_) { + for (const auto& [unused, kvs] : data_) { size += kvs.size(); } return size; diff --git a/mbo/file/ini/ini_file.h b/mbo/file/ini/ini_file.h index 4f96cbd5..e50cb11c 100644 --- a/mbo/file/ini/ini_file.h +++ b/mbo/file/ini/ini_file.h @@ -92,7 +92,7 @@ class IniFile { absl::btree_set GetGroups() const { absl::btree_set result; - for (const auto& [group, _] : data_) { + for (const auto& [group, unused] : data_) { result.emplace(group); } return result; diff --git a/mbo/hash/hash_benchmark.cc b/mbo/hash/hash_benchmark.cc index 6dd46705..09b5a4aa 100644 --- a/mbo/hash/hash_benchmark.cc +++ b/mbo/hash/hash_benchmark.cc @@ -269,14 +269,14 @@ std::size_t SampleLength(const LatencyDist& dist, double percentile) { const std::vector& ThroughputKeys(std::size_t dist_index, std::size_t bound_index) { static const std::array, kCdfPoints>, kLatencyDists.size()> kKeySets = [] { std::array, kCdfPoints>, kLatencyDists.size()> sets; - for (std::size_t d = 0; d < kLatencyDists.size(); ++d) { - const LatencyDist& dist = kLatencyDists[d]; - for (std::size_t b = 0; b < kCdfPoints; ++b) { + for (std::size_t dist_idx = 0; dist_idx < kLatencyDists.size(); ++dist_idx) { + const LatencyDist& dist = kLatencyDists[dist_idx]; + for (std::size_t point = 0; point < kCdfPoints; ++point) { // NOLINTNEXTLINE(cert-msc51-cpp,cert-msc32-c,bugprone-random-generator-seed): fixed, reproducible set std::mt19937_64 rng(0x1a7e9c1); - const double bound_pct = dist.cdf[b].first; - const auto bound_len = static_cast(dist.cdf[b].second); - std::vector& keys = sets[d][b]; + const double bound_pct = dist.cdf[point].first; + const auto bound_len = static_cast(dist.cdf[point].second); + std::vector& keys = sets[dist_idx][point]; keys.reserve(kLatencyKeys); for (std::size_t i = 0; i + 1 < kLatencyKeys; ++i) { const double draw = static_cast(rng()) / (static_cast(UINT64_MAX) + 1.0); diff --git a/mbo/testing/matchers.h b/mbo/testing/matchers.h index ead7fe76..222ee871 100644 --- a/mbo/testing/matchers.h +++ b/mbo/testing/matchers.h @@ -168,10 +168,10 @@ namespace testing_internal { // `IsElementOf`). Not exposed at namespace scope because the natural calling convention puts // the projection on the right of `EXPECT_THAT`, inside another matcher. template -inline std::vector AllKeys(const Map& m) { +inline std::vector AllKeys(const Map& map) { std::vector keys; - keys.reserve(m.size()); - for (const auto& kv : m) { + keys.reserve(map.size()); + for (const auto& kv : map) { keys.push_back(kv.first); } return keys; @@ -180,10 +180,10 @@ inline std::vector AllKeys(const Map& m) { // Returns the mapped values of an associative container as a `std::vector` in // iteration order. Internal: see `AllKeys` above. template -inline std::vector AllValues(const Map& m) { +inline std::vector AllValues(const Map& map) { std::vector values; - values.reserve(m.size()); - for (const auto& kv : m) { + values.reserve(map.size()); + for (const auto& kv : map) { values.push_back(kv.second); } return values; @@ -193,22 +193,22 @@ inline std::vector AllValues(const Map& m) { // Matcher that asserts the value-under-test equals at least one key of `map`. // -// std::map m = {{1, "a"}, {2, "b"}}; -// EXPECT_THAT(key, IsKeyOf(m)); +// std::map map = {{1, "a"}, {2, "b"}}; +// EXPECT_THAT(key, IsKeyOf(map)); template -inline auto IsKeyOf(const Map& m) { - auto keys = testing_internal::AllKeys(m); +inline auto IsKeyOf(const Map& map) { + auto keys = testing_internal::AllKeys(map); using KeysContainer = decltype(keys); return testing_internal::IsElementOfMatcher(std::move(keys), "is a key of", "is not a key of"); } // Matcher that asserts the value-under-test equals at least one mapped value of `map`. // -// std::map m = {{1, "a"}, {2, "b"}}; -// EXPECT_THAT(value, IsValueOf(m)); +// std::map map = {{1, "a"}, {2, "b"}}; +// EXPECT_THAT(value, IsValueOf(map)); template -inline auto IsValueOf(const Map& m) { - auto values = testing_internal::AllValues(m); +inline auto IsValueOf(const Map& map) { + auto values = testing_internal::AllValues(map); using ValuesContainer = decltype(values); return testing_internal::IsElementOfMatcher(std::move(values), "is a value of", "is not a value of"); } diff --git a/mbo/testing/matchers_test.cc b/mbo/testing/matchers_test.cc index 82612194..4c9b38f2 100644 --- a/mbo/testing/matchers_test.cc +++ b/mbo/testing/matchers_test.cc @@ -304,44 +304,44 @@ TEST_F(MatcherTest, IsElementOfEmptyDescriptions) { } TEST_F(MatcherTest, IsKeyAndIsValueOfMap) { - const std::map m{{1, "a"}, {2, "b"}}; - EXPECT_THAT(1, IsKeyOf(m)); - EXPECT_THAT(2, IsKeyOf(m)); - EXPECT_THAT(3, Not(IsKeyOf(m))); - EXPECT_THAT("a", IsValueOf(m)); - EXPECT_THAT("b", IsValueOf(m)); - EXPECT_THAT("z", Not(IsValueOf(m))); + const std::map map{{1, "a"}, {2, "b"}}; + EXPECT_THAT(1, IsKeyOf(map)); + EXPECT_THAT(2, IsKeyOf(map)); + EXPECT_THAT(3, Not(IsKeyOf(map))); + EXPECT_THAT("a", IsValueOf(map)); + EXPECT_THAT("b", IsValueOf(map)); + EXPECT_THAT("z", Not(IsValueOf(map))); } TEST_F(MatcherTest, IsKeyAndIsValueOfEmptyMap) { - const std::map m; - EXPECT_THAT(0, Not(IsKeyOf(m))); - EXPECT_THAT(42, Not(IsKeyOf(m))); - EXPECT_THAT("", Not(IsValueOf(m))); - EXPECT_THAT("anything", Not(IsValueOf(m))); + const std::map map; + EXPECT_THAT(0, Not(IsKeyOf(map))); + EXPECT_THAT(42, Not(IsKeyOf(map))); + EXPECT_THAT("", Not(IsValueOf(map))); + EXPECT_THAT("anything", Not(IsValueOf(map))); } TEST_F(MatcherTest, IsKeyAndIsValueOfUnorderedMap) { - const std::unordered_map m{{1, "a"}, {2, "b"}}; - EXPECT_THAT(1, IsKeyOf(m)); - EXPECT_THAT(2, IsKeyOf(m)); - EXPECT_THAT(3, Not(IsKeyOf(m))); - EXPECT_THAT("a", IsValueOf(m)); - EXPECT_THAT("b", IsValueOf(m)); - EXPECT_THAT("z", Not(IsValueOf(m))); + const std::unordered_map map{{1, "a"}, {2, "b"}}; + EXPECT_THAT(1, IsKeyOf(map)); + EXPECT_THAT(2, IsKeyOf(map)); + EXPECT_THAT(3, Not(IsKeyOf(map))); + EXPECT_THAT("a", IsValueOf(map)); + EXPECT_THAT("b", IsValueOf(map)); + EXPECT_THAT("z", Not(IsValueOf(map))); } TEST_F(MatcherTest, IsKeyAndIsValueOfMultimap) { // multimap allows duplicate keys; from the membership orientation duplicate // entries are simply both present. - const std::multimap mm{{1, "a"}, {1, "b"}, {2, "c"}}; - EXPECT_THAT(1, IsKeyOf(mm)); - EXPECT_THAT(2, IsKeyOf(mm)); - EXPECT_THAT(3, Not(IsKeyOf(mm))); - EXPECT_THAT("a", IsValueOf(mm)); - EXPECT_THAT("b", IsValueOf(mm)); - EXPECT_THAT("c", IsValueOf(mm)); - EXPECT_THAT("z", Not(IsValueOf(mm))); + const std::multimap multi_map{{1, "a"}, {1, "b"}, {2, "c"}}; + EXPECT_THAT(1, IsKeyOf(multi_map)); + EXPECT_THAT(2, IsKeyOf(multi_map)); + EXPECT_THAT(3, Not(IsKeyOf(multi_map))); + EXPECT_THAT("a", IsValueOf(multi_map)); + EXPECT_THAT("b", IsValueOf(multi_map)); + EXPECT_THAT("c", IsValueOf(multi_map)); + EXPECT_THAT("z", Not(IsValueOf(multi_map))); } TEST_F(MatcherTest, IsKeyOfStringMap) { @@ -356,8 +356,8 @@ TEST_F(MatcherTest, IsKeyOfStringMap) { } TEST_F(MatcherTest, IsKeyOfDescriptions) { - const std::map m{{1, "a"}, {2, "b"}}; - const ::testing::Matcher matcher = IsKeyOf(m); + const std::map map{{1, "a"}, {2, "b"}}; + const ::testing::Matcher matcher = IsKeyOf(map); EXPECT_THAT(Describe(matcher), "is a key of {1, 2}"); EXPECT_THAT(DescribeNegation(matcher), "is not a key of {1, 2}"); EXPECT_THAT(MatchAndExplain(matcher, 1), Pair(true, "which equals element #0 (1)")); @@ -365,16 +365,16 @@ TEST_F(MatcherTest, IsKeyOfDescriptions) { } TEST_F(MatcherTest, IsKeyOfEmptyMapDescriptions) { - const std::map m; - const ::testing::Matcher matcher = IsKeyOf(m); + const std::map map; + const ::testing::Matcher matcher = IsKeyOf(map); EXPECT_THAT(Describe(matcher), "is a key of {}"); EXPECT_THAT(DescribeNegation(matcher), "is not a key of {}"); EXPECT_THAT(MatchAndExplain(matcher, 42), Pair(false, "")); } TEST_F(MatcherTest, IsValueOfDescriptions) { - const std::map m{{1, "a"}, {2, "b"}}; - const ::testing::Matcher matcher = IsValueOf(m); + const std::map map{{1, "a"}, {2, "b"}}; + const ::testing::Matcher matcher = IsValueOf(map); EXPECT_THAT(Describe(matcher), "is a value of {\"a\", \"b\"}"); EXPECT_THAT(DescribeNegation(matcher), "is not a value of {\"a\", \"b\"}"); EXPECT_THAT(MatchAndExplain(matcher, std::string{"a"}), Pair(true, "which equals element #0 (\"a\")")); @@ -382,8 +382,8 @@ TEST_F(MatcherTest, IsValueOfDescriptions) { } TEST_F(MatcherTest, IsValueOfEmptyMapDescriptions) { - const std::map m; - const ::testing::Matcher matcher = IsValueOf(m); + const std::map map; + const ::testing::Matcher matcher = IsValueOf(map); EXPECT_THAT(Describe(matcher), "is a value of {}"); EXPECT_THAT(DescribeNegation(matcher), "is not a value of {}"); EXPECT_THAT(MatchAndExplain(matcher, std::string{"anything"}), Pair(false, ""));