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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp:136
+    CheckFactories.registerCheck<VariableLengthCheck>(
+        "readability-variable-length");
   }
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:20
+
+const unsigned DefaultMinimumVariableNameLength = 3;
+const unsigned DefaultMinimumLoopCounterNameLength = 2;
----------------
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.)


================
Comment at: clang-tools-extra/clang-tidy/readability/VariableLengthCheck.cpp:24
+const char DefaultIgnoredLoopCounterNames[] = "^[ijk_]$";
+const char DefaultIgnoredVariableNames[] = "";
+
----------------
Ignored exception names? There's a fair amount of `catch (const exception &e)` code in the world: https://codesearch.isocpp.org/cgi-bin/cgi_ppsearch?q=catch%28const+exception+%26e%29&search=Search)


================
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";
----------------
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`).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97753/new/

https://reviews.llvm.org/D97753