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