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