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