Daniel599 marked 3 inline comments as done.
Daniel599 added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:868
+ if (CheckNewIdentifier != Idents.end() &&
+ CheckNewIdentifier->second->isKeyword(getLangOpts())) {
+ Failure.FixStatus = ShouldFixStatus::ConflictsWithKeyword;
----------------
aaron.ballman wrote:
> 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?
> ```
That's another type of bug, just like the one I found
https://bugs.llvm.org/show_bug.cgi?id=43306
I don't aim on solving all of them in one patch, my patch just fixes keywords.
Also, I don't think my patch makes the above situation worse.
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h:88
+
+ bool ShouldNotify() const {
+ return (FixStatus == ShouldFixStatus::ShouldFix ||
----------------
aaron.ballman wrote:
> 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.
I`m sorry, I didn't understand you.
I tried to keep the old behavior of "cannot fix inside macro" and that's why I
needed the method `ShouldNotify`.
Do you suggest another idea or other code flow?
================
Comment at: clang/include/clang/Basic/IdentifierTable.h:584
+ iterator find(StringRef Name) const { return HashTable.find(Name); }
+
----------------
aaron.ballman wrote:
> Is this actually needed? Pretty sure you can use `std::find(table.begin(),
> table.end(), Name);` at the call site (or something similar).
Yes, because std::find would be slower here compare to `HashTable.find`
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
