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:
> > 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?
================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:20
+
+const unsigned DefaultMinimumVariableNameLength = 3;
+const unsigned DefaultMinimumLoopCounterNameLength = 2;
----------------
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.
================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:26
+
+const char ErrorMessage[] = "%select{|exception |loop }0 variable name %1 is "
+ "too short, expected at least %2 characters";
----------------
0x8000-0000 wrote:
> aaron.ballman wrote:
> > It looks like there will be whitespace issues with this. If the variable is
> > a loop or exception, it'll have an extra space (`loop variable name`) and
> > if it's not, it will start with a leading space (` variable name`).
> This was recommended by a previous reviewer. Any alternative suggestion here?
I *think* the diagnostic will work if you write it like this:
`"%select{variable |exception variable |loop variable |parameter }0name %1 is
too short, expected at least %2 characters"`
================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:98
+
+ const StringRef VarName = StandaloneVar->getName();
+
----------------
================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:114
+
+ const StringRef VarName = ExceptionVarName->getName();
+ if ((VarName.size() >= MinimumExceptionNameLength) ||
----------------
================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:128
+
+ const StringRef VarName = LoopVar->getName();
+
----------------
================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:143
+
+ const StringRef VarName = ParamVar->getName();
+
----------------
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/readability-variable-length.cpp:44
+
+void longEnoughVariableNames(int n) // argument 'n' ignored by configuration
+{
----------------
What in the configuration causes `n` to be ignored?
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
