diff --git a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h --- a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h +++ b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h @@ -65,6 +65,24 @@ std::string Suffix; }; + /// This enum will be used in %select of the diagnostic message. + /// Each value below IgnoreFailureThreshold should have an error message. + enum class ShouldFixStatus { + ShouldFix, + ConflictsWithKeyword, /// The fixup will conflict with a language keyword, + /// so we can't fix it automatically. + ConflictsWithMacroDefinition, /// The fixup will conflict with a macro + /// definition, so we can't fix it + /// automatically. + + /// Values pass this threshold will be ignored completely + /// i.e no message, no fixup. + IgnoreFailureThreshold, + + InsideMacro, /// If the identifier was used or declared within a macro we + /// won't offer a fixup for safety reasons. + }; + /// Holds an identifier name check failure, tracking the kind of the /// identifer, its possible fixup and the starting locations of all the /// identifier usages. @@ -76,13 +94,19 @@ /// /// ie: if the identifier was used or declared within a macro we won't offer /// a fixup for safety reasons. - bool ShouldFix; + bool ShouldFix() const { return FixStatus == ShouldFixStatus::ShouldFix; } + + bool ShouldNotify() const { + return FixStatus < ShouldFixStatus::IgnoreFailureThreshold; + } + + ShouldFixStatus FixStatus = ShouldFixStatus::ShouldFix; /// A set of all the identifier usages starting SourceLocation, in /// their encoded form. llvm::DenseSet RawUsageLocs; - NamingCheckFailure() : ShouldFix(true) {} + NamingCheckFailure() = default; }; typedef std::pair NamingCheckId; diff --git a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp --- a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp @@ -691,10 +691,11 @@ if (!Failure.RawUsageLocs.insert(FixLocation.getRawEncoding()).second) return; - if (!Failure.ShouldFix) + if (!Failure.ShouldFix()) return; - Failure.ShouldFix = utils::rangeCanBeFixed(Range, SourceMgr); + if (!utils::rangeCanBeFixed(Range, SourceMgr)) + Failure.FixStatus = IdentifierNamingCheck::ShouldFixStatus::InsideMacro; } /// Convenience method when the usage to be added is a NamedDecl @@ -873,6 +874,16 @@ DeclarationNameInfo(Decl->getDeclName(), Decl->getLocation()) .getSourceRange(); + const IdentifierTable &Idents = Decl->getASTContext().Idents; + auto CheckNewIdentifier = Idents.find(Fixup); + if (CheckNewIdentifier != Idents.end()) { + const IdentifierInfo *Ident = CheckNewIdentifier->second; + if (Ident->isKeyword(getLangOpts())) + Failure.FixStatus = ShouldFixStatus::ConflictsWithKeyword; + else if (Ident->hasMacroDefinition()) + Failure.FixStatus = ShouldFixStatus::ConflictsWithMacroDefinition; + } + Failure.Fixup = std::move(Fixup); Failure.KindName = std::move(KindName); addUsage(NamingCheckFailures, Decl, Range); @@ -935,24 +946,35 @@ if (Failure.KindName.empty()) continue; - if (Failure.ShouldFix) { - auto Diag = diag(Decl.first, "invalid case style for %0 '%1'") - << Failure.KindName << Decl.second; - - for (const auto &Loc : Failure.RawUsageLocs) { - // We assume that the identifier name is made of one token only. This is - // always the case as we ignore usages in macros that could build - // identifier names by combining multiple tokens. - // - // For destructors, we alread take care of it by remembering the - // location of the start of the identifier and not the start of the - // tilde. - // - // Other multi-token identifiers, such as operators are not checked at - // all. - Diag << FixItHint::CreateReplacement( - SourceRange(SourceLocation::getFromRawEncoding(Loc)), - Failure.Fixup); + if (Failure.ShouldNotify()) { + auto Diag = + diag(Decl.first, + "invalid case style for %0 '%1'%select{|" // Case 0 is empty on + // purpose, because we + // intent to provide a + // fix + "; cannot be fixed because '%3' would conflict with a keyword|" + "; cannot be fixed because '%3' would conflict with a macro " + "definition}2") + << Failure.KindName << Decl.second + << static_cast(Failure.FixStatus) << Failure.Fixup; + + if (Failure.ShouldFix()) { + for (const auto &Loc : Failure.RawUsageLocs) { + // We assume that the identifier name is made of one token only. This + // is always the case as we ignore usages in macros that could build + // identifier names by combining multiple tokens. + // + // For destructors, we already take care of it by remembering the + // location of the start of the identifier and not the start of the + // tilde. + // + // Other multi-token identifiers, such as operators are not checked at + // all. + Diag << FixItHint::CreateReplacement( + SourceRange(SourceLocation::getFromRawEncoding(Loc)), + Failure.Fixup); + } } } } diff --git a/clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp b/clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp @@ -0,0 +1,27 @@ +// RUN: %check_clang_tidy %s readability-identifier-naming %t -- \ +// RUN: -config='{CheckOptions: [ \ +// RUN: {key: readability-identifier-naming.ParameterCase, value: lower_case} \ +// RUN: ]}' + +int func(int Break) { + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for parameter 'Break'; cannot be fixed because 'break' would conflict with a keyword + // CHECK-FIXES: {{^}}int func(int Break) {{{$}} + if (Break == 1) { + // CHECK-FIXES: {{^}} if (Break == 1) {{{$}} + return 2; + } + + return 0; +} + +#define foo 3 +int func2(int Foo) { + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for parameter 'Foo'; cannot be fixed because 'foo' would conflict with a macro definition + // CHECK-FIXES: {{^}}int func2(int Foo) {{{$}} + if (Foo == 1) { + // CHECK-FIXES: {{^}} if (Foo == 1) {{{$}} + return 2; + } + + return 0; +} diff --git a/clang/include/clang/Basic/IdentifierTable.h b/clang/include/clang/Basic/IdentifierTable.h --- a/clang/include/clang/Basic/IdentifierTable.h +++ b/clang/include/clang/Basic/IdentifierTable.h @@ -581,6 +581,8 @@ iterator end() const { return HashTable.end(); } unsigned size() const { return HashTable.size(); } + iterator find(StringRef Name) const { return HashTable.find(Name); } + /// Print some statistics to stderr that indicate how well the /// hashing is doing. void PrintStats() const;