[PATCH] D97753: [clang-tidy] Add a check for enforcing minimum length for variable names

[Nathan James via Phabricator via cfe-commits]

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