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 @@ -64,6 +64,14 @@ std::string Suffix; }; + enum class ShouldFixStatus { + ShouldFix = 0, + InsideMacro, /// if the identifier was used or declared within a macro we + /// won't offer a fixup for safety reasons. + ConflictsWithKeyword /// The fixup will conflict with a language keyword, so + /// we can't fix it automatically + }; + /// Holds an identifier name check failure, tracking the kind of the /// identifer, its possible fixup and the starting locations of all the /// identifier usages. @@ -75,13 +83,20 @@ /// /// 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::ShouldFix || + FixStatus == ShouldFixStatus::ConflictsWithKeyword); + } + + ShouldFixStatus FixStatus; /// A set of all the identifier usages starting SourceLocation, in /// their encoded form. llvm::DenseSet RawUsageLocs; - NamingCheckFailure() : ShouldFix(true) {} + NamingCheckFailure() : FixStatus(ShouldFixStatus::ShouldFix) {} }; 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 @@ -679,10 +679,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 @@ -861,6 +862,13 @@ DeclarationNameInfo(Decl->getDeclName(), Decl->getLocation()) .getSourceRange(); + auto &Idents = Decl->getASTContext().Idents; + auto CheckNewIdentifier = Idents.find(Fixup); + if (CheckNewIdentifier != Idents.end() && + CheckNewIdentifier->second->isKeyword(getLangOpts())) { + Failure.FixStatus = ShouldFixStatus::ConflictsWithKeyword; + } + Failure.Fixup = std::move(Fixup); Failure.KindName = std::move(KindName); addUsage(NamingCheckFailures, Decl, Range); @@ -923,24 +931,35 @@ if (Failure.KindName.empty()) continue; - if (Failure.ShouldFix) { - auto Diag = diag(Decl.first, "invalid case style for %0 '%1'") + if (Failure.ShouldNotify()) { + auto Diag = diag(Decl.first, "invalid case style for %0 '%1'%2") << 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.FixStatus == ShouldFixStatus::ConflictsWithKeyword) { + std::string CannotFixReason = + std::string(". Cannot be fixed because '") + Failure.Fixup + + "' would conflict with a language keyword"; + Diag << CannotFixReason; + } else { + Diag << ""; + } + + 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,15 @@ +// 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 language keyword + // CHECK-FIXES: {{^}}int func(int Break) {{{$}} + if (Break == 1) { + // CHECK-FIXES: {{^}} if (Break == 1) {{{$}} + return 2; + } + + return 0; +} \ No newline at end of file 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;