njames93 added a comment.
Please can you upload diffs with full context
<https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface>
or using arcanist
<https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-command-line>.
It may be worth adding a case where if the `Minimum<Type>NameLength` is set to
0, it won't check that type, or even bother registering its matchers.
It may be worth adding `unless(isImplicit())` to each `varDecl()` matcher, save
any nasties from compiler generated decls.
================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:23
+const unsigned DefaultMinimumExceptionNameLength = 2;
+const char DefaultIgnoredLoopCounterNames[] = "i;j;k;";
+
----------------
If you make this regex this could be changed to `"^[ijk]$"`.
================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:30
+ DefaultMinimumVariableNameLength)},
+ MinimumLoopCounterNameLength{Options.get(
+ "MinimumLoopCounterNameLength",
DefaultMinimumLoopCounterNameLength)},
----------------
For better or worse, we typically use parentheses instead of braced
initialization for the member init list.
================
Comment at:
clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:43-50
+ Options.store(Opts, "MinimumVariableNameLength",
+ DefaultMinimumVariableNameLength);
+ Options.store(Opts, "MinimumLoopCounterNameLength",
+ DefaultMinimumLoopCounterNameLength);
+ Options.store(Opts, "MinimumExceptionNameLength",
+ DefaultMinimumExceptionNameLength);
+ Options.store(Opts, "IgnoredLoopCounterNames",
----------------
You shouldn't be storing the default value, you should be storing the value
read from config.
================
Comment at:
clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:54-55
+void VariableLengthCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ varDecl(hasParent(declStmt(hasParent(forStmt())))).bind("loopVar"),
this);
+ Finder->addMatcher(varDecl(hasParent(cxxCatchStmt())).bind("exceptionVar"),
----------------
This code will match i, j and k in this example
```lang=c
for (int i = 4, j = 5;;)
int k = 5;
```
It may be better to use this:
```lang=c++
// To match both i and j
forStmt(hasLoopInit(declStmt(forEach(varDecl().bind("loopVar")))))
// To only match i
forStmt(hasLoopInit(declStmt(containsDeclaration(0,
varDecl().bind("loopVar")))))
```
================
Comment at:
clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:58-62
+ Finder->addMatcher(
+ varDecl(unless(anyOf(hasParent(declStmt(hasParent(forStmt()))),
+ hasParent(cxxCatchStmt()))))
+ .bind("standaloneVar"),
+ this);
----------------
This will miss the match for `k` in the same example:
```lang=c
for (int i = 4, j = 5;;)
int k = 5;
```
Gotta scratch my head for this one though. Though like the case before its
rather unlikely to show up in code so not the end of the world.
Also varDecl will match parameters, is this intended can you add tests and
explain in documentation.
If its unintended putting `unless(parmVarDecl())` in the matcher disable that.
================
Comment at:
clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:72-74
+ diag(StandaloneVar->getLocation(),
+ "variable name %0 is too short, expected at least %1 characters")
+ << StandaloneVar << MinimumVariableNameLength;
----------------
This message can(should) be reused for all warnings with select.
Obviously you'd put that in a char arrray and the reference it for the next 2
diags.
================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.h:20
+
+/// Warns about variable names whose length is too short
+///
----------------
Full-stop/period at the end of comments.
================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.h:36
+
+ std::unordered_set<std::string> IgnoredLoopCounterNames;
+};
----------------
I feel like regex is better suited for this purpose and just ignore any loop
variables that match the regex.
Doing this will require storing a string as well as the regex for the purpose
of `storeOptions`.
Repository:
rG LLVM Github Monorepo
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
