From e477988309dbde214a6d16ec690a416882714aac Mon Sep 17 00:00:00 2001 From: Daniel Date: Wed, 30 Oct 2019 14:15:47 -0400 Subject: [PATCH] Fix readability-identifier-naming to prevent variables becoming keywords. Do not provide a fix-it when clang-tidy encounters a name that would become a keyword. --- .../readability/IdentifierNamingCheck.cpp | 62 +++++++++++++++------- .../clang-tidy/readability/IdentifierNamingCheck.h | 28 +++++++++- clang/include/clang/Basic/IdentifierTable.h | 2 + 3 files changed, 70 insertions(+), 22 deletions(-) diff --git a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp index 3228940..5b78155 100644 --- a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp @@ -691,10 +691,11 @@ static void addUsage(IdentifierNamingCheck::NamingCheckFailureMap &Failures, 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 @@ void IdentifierNamingCheck::check(const MatchFinder::MatchResult &Result) { 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 @@ void IdentifierNamingCheck::onEndOfTranslationUnit() { 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/clang-tidy/readability/IdentifierNamingCheck.h b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h index 250dc36..e72ae4e 100644 --- a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h +++ b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h @@ -65,6 +65,24 @@ public: 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 @@ public: /// /// 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/include/clang/Basic/IdentifierTable.h b/clang/include/clang/Basic/IdentifierTable.h index 37d7198..7aa1d07 100644 --- a/clang/include/clang/Basic/IdentifierTable.h +++ b/clang/include/clang/Basic/IdentifierTable.h @@ -581,6 +581,8 @@ public: 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; -- 2.7.4