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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
