[PATCH] D76606: [clang-tidy] Warn on invalid "case" configurations for readability-identifier-naming

[Nathan James via Phabricator via cfe-commits]

njames93 created this revision.
njames93 added reviewers: aaron.ballman, alexfh.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.
njames93 marked 2 inline comments as done.
njames93 added a project: clang-tools-extra.
njames93 added a comment.

I'm not too sure how many other checks are like this, but I feel that this functionality could maybe be brought out of this check and into the Options to let more checks that take enum like configurations to use it.



================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:154
+      llvm::errs() << "warning: Invalid case style '" << CaseValue
+                   << "' for readability-identifier-naming." << CaseOptionName;
+      constexpr StringRef AcceptibleNames[] = {
----------------
Unsure about hardcoding the check-name as it could be ran under an alias. However there is no way for a ClangTidyCheck to get the name its ran as. `ClangTidyCheck::CheckName` is `private`, maybe a protected getter would be wise.


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:177
+      if (EditDistance < 3)
+        llvm::errs() << ": did you mean '" << Closest << "'\n";
+      else
----------------
I feel its safer not assuming the fix and instead letting the user modify their configuration, WDYT?


Issue a warning (and potential fix hint) when an invalid case value is supplied to readability-identifier-naming configuration.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76606

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-case-validation.cpp


Index: clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-case-validation.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-identifier-naming-case-validation.cpp
@@ -0,0 +1,16 @@
+// RUN: clang-tidy %s -checks=readability-identifier-naming \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: readability-identifier-naming.FunctionCase, value: camelback}, \
+// RUN:   {key: readability-identifier-naming.VariableCase, value: camelBack}, \
+// RUN:   {key: readability-identifier-naming.ClassCase, value: UUPER_CASE}, \
+// RUN:   {key: readability-identifier-naming.StructCase, value: CAMEL}, \
+// RUN:   {key: readability-identifier-naming.EnumCase, value: AnY_cASe}, \
+// RUN:   ]}" 2>&1 | FileCheck %s --implicit-check-not warning
+
+
+// CHECK-DAG: warning: Invalid case style 'camelback' for readability-identifier-naming.FunctionCase: did you mean 'camelBack'{{$}}
+// CHECK-DAG: warning: Invalid case style 'UUPER_CASE' for readability-identifier-naming.ClassCase: did you mean 'UPPER_CASE'{{$}}
+// Don't try to suggest an alternative for 'CAMEL'
+// CHECK-DAG: warning: Invalid case style 'CAMEL' for readability-identifier-naming.StructCase{{$}}
+// This fails on the EditDistance, but as it matches ignoring case suggest the correct value
+// CHECK-DAG: warning: Invalid case style 'AnY_cASe' for readability-identifier-naming.EnumCase: did you mean 'aNy_CasE'{{$}}
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -95,6 +95,15 @@
 };
 
 #undef NAMING_KEYS
+
+#define CASE_KEYS(m) \
+    m("aNy_CasE", CT_AnyCase) \
+    m("lower_case", CT_LowerCase) \
+    m("UPPER_CASE", CT_UpperCase) \
+    m("camelBack", CT_CamelBack) \
+    m("CamelCase", CT_CamelCase) \
+    m("Camel_Snake_Case", CT_CamelSnakeCase) \
+    m("camel_Snake_Back", CT_CamelSnakeBack)
 // clang-format on
 
 namespace {
@@ -131,19 +140,44 @@
       IgnoreMainLikeFunctions(Options.get("IgnoreMainLikeFunctions", 0)) {
   auto const fromString = [](StringRef Str) {
     return llvm::StringSwitch<llvm::Optional<CaseType>>(Str)
-        .Case("aNy_CasE", CT_AnyCase)
-        .Case("lower_case", CT_LowerCase)
-        .Case("UPPER_CASE", CT_UpperCase)
-        .Case("camelBack", CT_CamelBack)
-        .Case("CamelCase", CT_CamelCase)
-        .Case("Camel_Snake_Case", CT_CamelSnakeCase)
-        .Case("camel_Snake_Back", CT_CamelSnakeBack)
-        .Default(llvm::None);
+#define CASE(NAME, ENUM) .Case(NAME, ENUM)
+        CASE_KEYS(CASE)
+#undef CASE
+            .Default(llvm::None);
   };
-
   for (auto const &Name : StyleNames) {
-    auto const caseOptional =
-        fromString(Options.get((Name + "Case").str(), ""));
+    auto CaseOptionName = (Name + "Case").str();
+    auto CaseValue = Options.get(CaseOptionName, "");
+    auto const caseOptional = fromString(CaseValue);
+    if (!caseOptional && !CaseValue.empty()) {
+      llvm::errs() << "warning: Invalid case style '" << CaseValue
+                   << "' for readability-identifier-naming." << CaseOptionName;
+      constexpr StringRef AcceptibleNames[] = {
+#define CASE(NAME, ENUM) NAME,
+          CASE_KEYS(CASE)
+#undef CASE
+      };
+      StringRef Closest = "";
+      unsigned EditDistance = -1u;
+      for (StringRef CheckMatch : AcceptibleNames) {
+        if (CheckMatch.equals_lower(
+                CaseValue)) { // bad case just suggest the fix straight away
+          EditDistance = 0;
+          Closest = CheckMatch;
+          break;
+        }
+        unsigned Distance =
+            llvm::StringRef(CaseValue).edit_distance(CheckMatch);
+        if (Distance < EditDistance) {
+          EditDistance = Distance;
+          Closest = CheckMatch;
+        }
+      }
+      if (EditDistance < 3)
+        llvm::errs() << ": did you mean '" << Closest << "'\n";
+      else
+        llvm::errs() << '\n';
+    }
     auto prefix = Options.get((Name + "Prefix").str(), "");
     auto postfix = Options.get((Name + "Suffix").str(), "");
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D76606.252008.patch
Type: text/x-patch
Size: 4286 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200323/2b5886be/attachment.bin>