[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Florin Iucha via Phabricator via cfe-commits]

0x8000-0000 marked an inline comment as done.
0x8000-0000 added a comment.

Added regex for exception names and rebased onto current 'main' branch.



================
Comment at: clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp:136
+    CheckFactories.registerCheck<VariableLengthCheck>(
+        "readability-variable-length");
   }
----------------
aaron.ballman wrote:
> Is there a reason this should be restricted to variables? For example, wouldn't the same functionality be useful for type names, or dare I say it, even macro names? I'm wondering if this should be `readability-identifier-length` to be more generic.
I consider those to be in separate namespaces. I suppose we could have a single checker with multiple rules, but on the other hand I don't want to combine too many things into one, just because they share the "compare length" dimension.


================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:20
+
+const unsigned DefaultMinimumVariableNameLength = 3;
+const unsigned DefaultMinimumLoopCounterNameLength = 2;
----------------
aaron.ballman wrote:
> Should it be possible to enforce parameters differently than local variables? (It seems a bit odd that we'd allow you to specify exception variable length separate from loops but not function parameters separate from locals.)
Exceptions and Loops historically are "throw-away" / "don't care" / "one-off" variables. Parameter names are names, and they are the interface between the outside of the routine and the processing inside; other than historical, I don't see good arguments (sic) to allow single-letter parameter names.

Note that this check will be quite noisy on a legacy code base, and people will find little reason to invest the work to remove the warnings. But if somebody starts something new and want to enforce some quality attributes, it is the right tool at the right time. There will be new projects starting in 2021 and 2022 that could benefit from this, I hope.


================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:26
+
+const char ErrorMessage[] = "%select{|exception |loop }0 variable name %1 is "
+                            "too short, expected at least %2 characters";
----------------
aaron.ballman wrote:
> It looks like there will be whitespace issues with this. If the variable is a loop or exception, it'll have an extra space (`loop  variable name`) and if it's not, it will start with a leading space (` variable name`).
This was recommended by a previous reviewer. Any alternative suggestion here?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97753/new/

https://reviews.llvm.org/D97753