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