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

[Florin Iucha via Phabricator via cfe-commits]

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:
> > > 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.
I can do the rename, but my intuition (informed somewhat by a few years in the industry) is that outside of T as template argument and X for X-Macros, there aren't many single or even two letter class names or structures out there outside of intentionally obfuscated code - thus I don't think there will be a need to have readability-identifier-length produced.

To avoid potential churn and the hassle with deprecation I'll make the change though.


================
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:
> 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?
Sorry about that; fixed in next patch.


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

https://reviews.llvm.org/D97753