[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 inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp:136
+    CheckFactories.registerCheck<VariableLengthCheck>(
+        "readability-variable-length");
   }
----------------
aaron.ballman wrote:
> 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?
Right - that's a good point. But I'm wondering the other way; maybe the bigger check will subsume this one, and this one will become just an alias for the bigger check?

So I'm -0.1 on using the "bigger name" for the limited functionality, but if one more vote comes in saying to go readability-identifier-length I'll rename this (and add explicitly the scope limits in the documentation.)


================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:20
+
+const unsigned DefaultMinimumVariableNameLength = 3;
+const unsigned DefaultMinimumLoopCounterNameLength = 2;
----------------
aaron.ballman wrote:
> 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.
Indeed - I have changed the code to meet this capability; now parameters are controlled independently of the regular variables.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp:44
+
+void longEnoughVariableNames(int n) // argument 'n' ignored by configuration
+{
----------------
aaron.ballman wrote:
> What in the configuration causes `n` to be ignored?
It is ignored by the default configuration. Search for "DefaultIgnoredParameterNames" above.


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

https://reviews.llvm.org/D97753