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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp:136
+    CheckFactories.registerCheck<VariableLengthCheck>(
+        "readability-variable-length");
   }
----------------
0x8000-0000 wrote:
> 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.
I see where you're coming from, but I come down on the other side. Running a check is expensive (especially on Windows where process creation can be really slow), so having multiple checks that traverse the AST gives worse performance than having a single check that only traverses the AST once. I'd rather see related functionality grouped together and use options to control behavior when it's a natural fit to do so.

I should note that I don't mean *you* have to implement this other functionality (as part of this patch or otherwise). Just that I think we should leave the check name ambiguous enough that we could grow it to do that work in the future.

WDYT?


================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:20
+
+const unsigned DefaultMinimumVariableNameLength = 3;
+const unsigned DefaultMinimumLoopCounterNameLength = 2;
----------------
0x8000-0000 wrote:
> 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.
> 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.

I largely agree with you, but there are definitely cases where single-letter parameter names are extremely common. For example, coordinates are often `x`, `y`, and `z`, colors are often `r`, `g`, and `b` (among many others), and physics applications often do things like `f = m * a` with their variables. Also, there are cases where the parameter names are often not super meaningful, like with functions `min` and `max` so the parameter names may be quite short.


================
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";
----------------
0x8000-0000 wrote:
> 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?
I *think* the diagnostic will work if you write it like this:
`"%select{variable |exception variable |loop variable |parameter }0name %1 is too short, expected at least %2 characters"`



================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:98
+
+    const StringRef VarName = StandaloneVar->getName();
+
----------------



================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:114
+
+    const StringRef VarName = ExceptionVarName->getName();
+    if ((VarName.size() >= MinimumExceptionNameLength) ||
----------------



================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:128
+
+    const StringRef VarName = LoopVar->getName();
+
----------------



================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:143
+
+    const StringRef VarName = ParamVar->getName();
+
----------------



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp:44
+
+void longEnoughVariableNames(int n) // argument 'n' ignored by configuration
+{
----------------
What in the configuration causes `n` to be ignored?


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

https://reviews.llvm.org/D97753