[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:
> > 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.)
> 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?

The downside to that approach is that the alias is a bit confusing until its deprecation period ends and we remove it. However, that's not a huge downside., so I don't insist on the name change if you're resistant to it.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp:44
+
+void longEnoughVariableNames(int n) // argument 'n' ignored by configuration
+{
----------------
0x8000-0000 wrote:
> aaron.ballman wrote:
> > What in the configuration causes `n` to be ignored?
> It is ignored by the default configuration. Search for "DefaultIgnoredParameterNames" above.
Ah, the comment tripped me up -- can you say `ignored via default configuration` like below to make it more clear?


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

https://reviews.llvm.org/D97753