Skip to content

Clear readability-identifier-length by renaming, not by configuring - #276

Merged
helly25 merged 2 commits into
mainfrom
clang_tidy_identifier_length
Aug 8, 2026
Merged

Clear readability-identifier-length by renaming, not by configuring#276
helly25 merged 2 commits into
mainfrom
clang_tidy_identifier_length

Conversation

@helly25

@helly25 helly25 commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Sixth clang-tidy triage PR. Clears all 39 readability-identifier-length findings. .clang-tidy is untouched.

The plan changed once I looked at the code

We had agreed to widen IgnoredParameterNames with m, on the understanding it was the gtest matcher idiom. It is not. In mbo/testing/matchers.h the m is a map:

template<typename Map> inline auto IsKeyOf(const Map& m);
template<typename Map> std::vector<typename Map::key_type> AllKeys(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 clearer and shorter than teaching the linter to ignore a letter across the whole repository — so no config change is needed, and the check keeps working everywhere else.

The rest, case by case

where change
diff_benchmark.cc toto_pos, jpos
hash_benchmark.cc ddist_idx, bpoint
ini_file.{h,cc} _unused (the ignored half of a structured binding)
diff_myers.cc waived for the file — see below

The hash_benchmark.cc rename is worth a note: the obvious choice, ddist, collides with const LatencyDist& dist in the same loop and would have silently produced dist = kLatencyDists[dist]. Hence dist_idx.

diff_myers.cc keeps its names because they are the notation of Myers' paper: n/m 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. Expanding those to three characters would break the correspondence to the paper that makes the algorithm checkable. The waiver is a single file-scoped region with that rationale, rather than 13 individual comments.

Test

  • readability-identifier-length reports zero across every affected file.
  • The waiver is narrow, not a blanket: 20 findings from other checks still fire in diff_myers.cc.
  • bazel test --config=clang //...109/109 pass.
  • pre-commit run -a green.

helly25 and others added 2 commits August 8, 2026 21:25
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>
@helly25
helly25 requested a review from Fab-Cat August 8, 2026 21:09
@helly25
helly25 enabled auto-merge (squash) August 8, 2026 21:21
@helly25
helly25 merged commit e70e580 into main Aug 8, 2026
23 checks passed
@helly25
helly25 deleted the clang_tidy_identifier_length branch August 8, 2026 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants