[PATCH] D68539: [clang-tidy] fix for readability-identifier-naming incorrectly fixes variables which become keywords

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:865
 
+      IdentifierTable &Idents = Decl->getASTContext().Idents;
+      auto CheckNewIdentifier = Idents.find(Fixup);
----------------
Can this be `const IdentifierTable&`?


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:868
+      if (CheckNewIdentifier != Idents.end() &&
+          CheckNewIdentifier->second->isKeyword(getLangOpts())) {
+        Failure.FixStatus = ShouldFixStatus::ConflictsWithKeyword;
----------------
What if changing it would switch to using a macro instead of a keyword? e.g.,
```
#define foo 12

void func(int Foo); // Changing Foo to foo would be bad, no?
```


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:940
+        std::string CannotFixReason =
+            std::string(". Cannot be fixed because '") + Failure.Fixup +
+            "' would conflict with a language keyword";
----------------
Diagnostics are not full sentences, so the full stop and capitalization are wrong here. I think this should be combined with the diagnostic above using a `%select`.
```
auto Diag = diag(Decl.first, "invalid case style for %0 '%1'%select{; cannot be fixed because '%3' would conflict with a keyword|}2")
                  << Failure.KindName << Decl.second << (Failure.FixStatus != ShouldFixStatus::ConflictsWithKeyword) << Failure.Fixup;
```


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h:69
+    ShouldFix,
+    InsideMacro, /// if the identifier was used or declared within a macro we
+                 /// won't offer a fixup for safety reasons.
----------------
if -> If


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h:72
+    ConflictsWithKeyword /// The fixup will conflict with a language keyword, so
+                         /// we can't fix it automatically
+  };
----------------
Missing a full stop at the end of the comment.


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h:88
+
+    bool ShouldNotify() const {
+      return (FixStatus == ShouldFixStatus::ShouldFix ||
----------------
This seems a bit confusing to me. It seems like the reason to not generate a fixit is being used to determine whether to diagnose at all, which seems incorrect despite sort of being what the old code did.


================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h:89
+    bool ShouldNotify() const {
+      return (FixStatus == ShouldFixStatus::ShouldFix ||
+              FixStatus == ShouldFixStatus::ConflictsWithKeyword);
----------------
Drop the parens.


================
Comment at: clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp:16
+}
\ No newline at end of file

----------------
Please add the newline to the end of the file.


================
Comment at: clang/include/clang/Basic/IdentifierTable.h:584
 
+  iterator find(StringRef Name) const { return HashTable.find(Name); }
+
----------------
Is this actually needed? Pretty sure you can use `std::find(table.begin(), table.end(), Name);` at the call site (or something similar).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68539