aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM aside from a minor nit.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:869-873
+ if (Ident->isKeyword(getLangOpts())) {
+ Failure.FixStatus = ShouldFixStatus::ConflictsWithKeyword;
+ } else if (Ident->hasMacroDefinition()) {
+ Failure.FixStatus = ShouldFixStatus::ConflictsWithMacroDefinition;
+ }
----------------
You can elide the curly braces.
================
Comment at:
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:868
+ if (CheckNewIdentifier != Idents.end() &&
+ CheckNewIdentifier->second->isKeyword(getLangOpts())) {
+ Failure.FixStatus = ShouldFixStatus::ConflictsWithKeyword;
----------------
Daniel599 wrote:
> Daniel599 wrote:
> > 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.
> Regarding your comment about macro name, I can fix it using
> `IdentifierInfo::hasMacroDefinition`.
> Should I fix it in this patch? I`ll add another value to `ShouldFixStatus`
> and another error message
Thanks for fixing this!
================
Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h:88
+
+ bool ShouldNotify() const {
+ return (FixStatus == ShouldFixStatus::ShouldFix ||
----------------
Daniel599 wrote:
> 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?
I see now that the reason we don't diagnose is because the construct is within
a macro, which is a pretty reasonable thing to do, so I'm no longer confused.
:-)
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
