Author: Edwin Vane Date: 2024-05-07T21:06:51+02:00 New Revision: b1bc1dbea6d0423813bb73d625c6eedc040007ed
URL: https://github.com/llvm/llvm-project/commit/b1bc1dbea6d0423813bb73d625c6eedc040007ed DIFF: https://github.com/llvm/llvm-project/commit/b1bc1dbea6d0423813bb73d625c6eedc040007ed.diff LOG: [clang-tidy] Refactor how NamedDecl are renamed (#88735) The handling of renaming failures and multiple usages related to those failures is currently spread over several functions. Identifying the failure NamedDecl for a given usage is also duplicated, once when creating failures and again when identify usages. There are currently two ways to a failed NamedDecl from a usage: use the canonical decl or use the overridden method. With new methods about to be added, a cleanup was in order. The data flow is simplified as follows: * The visitor always forwards NamedDecls to addUsage(NamedDecl). * addUsage(NamedDecl) determines the failed NamedDecl and determines potential new names based on that failure. Usages are registered using addUsage(NamingCheckId). * addUsage(NamingCheckId) is now protected and its single responsibility is maintaining the integrity of the failure/usage map. Added: Modified: clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp index f6714d056518da..53956661d57d13 100644 --- a/clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/ReservedIdentifierCheck.cpp @@ -178,8 +178,11 @@ std::optional<RenamerClangTidyCheck::FailureInfo> ReservedIdentifierCheck::getDeclFailureInfo(const NamedDecl *Decl, const SourceManager &) const { assert(Decl && Decl->getIdentifier() && !Decl->getName().empty() && - !Decl->isImplicit() && "Decl must be an explicit identifier with a name."); + // Implicit identifiers cannot fail. + if (Decl->isImplicit()) + return std::nullopt; + return getFailureInfoImpl( Decl->getName(), isa<TranslationUnitDecl>(Decl->getDeclContext()), /*IsMacro = */ false, getLangOpts(), Invert, AllowedIdentifiers); diff --git a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp index dc30531ebda0e9..27a12bfc580682 100644 --- a/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp @@ -1374,6 +1374,10 @@ IdentifierNamingCheck::getFailureInfo( std::optional<RenamerClangTidyCheck::FailureInfo> IdentifierNamingCheck::getDeclFailureInfo(const NamedDecl *Decl, const SourceManager &SM) const { + // Implicit identifiers cannot be renamed. + if (Decl->isImplicit()) + return std::nullopt; + SourceLocation Loc = Decl->getLocation(); const FileStyle &FileStyle = getStyleForFile(SM.getFilename(Loc)); if (!FileStyle.isActive()) diff --git a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp index 962a243ce94d48..f5ed617365403a 100644 --- a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp +++ b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.cpp @@ -61,6 +61,7 @@ struct DenseMapInfo<clang::tidy::RenamerClangTidyCheck::NamingCheckId> { namespace clang::tidy { namespace { + class NameLookup { llvm::PointerIntPair<const NamedDecl *, 1, bool> Data; @@ -78,6 +79,7 @@ class NameLookup { operator bool() const { return !hasMultipleResolutions(); } const NamedDecl *operator*() const { return getDecl(); } }; + } // namespace static const NamedDecl *findDecl(const RecordDecl &RecDecl, @@ -91,6 +93,44 @@ static const NamedDecl *findDecl(const RecordDecl &RecDecl, return nullptr; } +/// Returns the function that \p Method is overridding. If There are none or +/// multiple overrides it returns nullptr. If the overridden function itself is +/// overridding then it will recurse up to find the first decl of the function. +static const CXXMethodDecl *getOverrideMethod(const CXXMethodDecl *Method) { + if (Method->size_overridden_methods() != 1) + return nullptr; + + while (true) { + Method = *Method->begin_overridden_methods(); + assert(Method && "Overridden method shouldn't be null"); + unsigned NumOverrides = Method->size_overridden_methods(); + if (NumOverrides == 0) + return Method; + if (NumOverrides > 1) + return nullptr; + } +} + +static bool hasNoName(const NamedDecl *Decl) { + return !Decl->getIdentifier() || Decl->getName().empty(); +} + +static const NamedDecl *getFailureForNamedDecl(const NamedDecl *ND) { + const auto *Canonical = cast<NamedDecl>(ND->getCanonicalDecl()); + if (Canonical != ND) + return Canonical; + + if (const auto *Method = dyn_cast<CXXMethodDecl>(ND)) { + if (const CXXMethodDecl *Overridden = getOverrideMethod(Method)) + Canonical = cast<NamedDecl>(Overridden->getCanonicalDecl()); + + if (Canonical != ND) + return Canonical; + } + + return ND; +} + /// Returns a decl matching the \p DeclName in \p Parent or one of its base /// classes. If \p AggressiveTemplateLookup is `true` then it will check /// template dependent base classes as well. @@ -132,24 +172,6 @@ static NameLookup findDeclInBases(const CXXRecordDecl &Parent, return NameLookup(Found); // If nullptr, decl wasn't found. } -/// Returns the function that \p Method is overridding. If There are none or -/// multiple overrides it returns nullptr. If the overridden function itself is -/// overridding then it will recurse up to find the first decl of the function. -static const CXXMethodDecl *getOverrideMethod(const CXXMethodDecl *Method) { - if (Method->size_overridden_methods() != 1) - return nullptr; - - while (true) { - Method = *Method->begin_overridden_methods(); - assert(Method && "Overridden method shouldn't be null"); - unsigned NumOverrides = Method->size_overridden_methods(); - if (NumOverrides == 0) - return Method; - if (NumOverrides > 1) - return nullptr; - } -} - namespace { /// Callback supplies macros to RenamerClangTidyCheck::checkMacro @@ -192,10 +214,6 @@ class RenamerClangTidyVisitor : Check(Check), SM(SM), AggressiveDependentMemberLookup(AggressiveDependentMemberLookup) {} - static bool hasNoName(const NamedDecl *Decl) { - return !Decl->getIdentifier() || Decl->getName().empty(); - } - bool shouldVisitTemplateInstantiations() const { return true; } bool shouldVisitImplicitCode() const { return false; } @@ -246,29 +264,10 @@ class RenamerClangTidyVisitor } bool VisitNamedDecl(NamedDecl *Decl) { - if (hasNoName(Decl)) - return true; - - const auto *Canonical = cast<NamedDecl>(Decl->getCanonicalDecl()); - if (Canonical != Decl) { - Check->addUsage(Canonical, Decl->getLocation(), SM); - return true; - } - - // Fix overridden methods - if (const auto *Method = dyn_cast<CXXMethodDecl>(Decl)) { - if (const CXXMethodDecl *Overridden = getOverrideMethod(Method)) { - Check->addUsage(Overridden, Method->getLocation(), SM); - return true; // Don't try to add the actual decl as a Failure. - } - } - - // Ignore ClassTemplateSpecializationDecl which are creating duplicate - // replacements with CXXRecordDecl. - if (isa<ClassTemplateSpecializationDecl>(Decl)) - return true; - - Check->checkNamedDecl(Decl, SM); + SourceRange UsageRange = + DeclarationNameInfo(Decl->getDeclName(), Decl->getLocation()) + .getSourceRange(); + Check->addUsage(Decl, UsageRange, SM); return true; } @@ -413,82 +412,97 @@ void RenamerClangTidyCheck::registerPPCallbacks( std::make_unique<RenamerClangTidyCheckPPCallbacks>(SM, this)); } -void RenamerClangTidyCheck::addUsage( - const RenamerClangTidyCheck::NamingCheckId &Decl, SourceRange Range, - const SourceManager &SourceMgr) { +std::pair<RenamerClangTidyCheck::NamingCheckFailureMap::iterator, bool> +RenamerClangTidyCheck::addUsage( + const RenamerClangTidyCheck::NamingCheckId &FailureId, + SourceRange UsageRange, const SourceManager &SourceMgr) { // Do nothing if the provided range is invalid. - if (Range.isInvalid()) - return; + if (UsageRange.isInvalid()) + return {NamingCheckFailures.end(), false}; - // If we have a source manager, use it to convert to the spelling location for - // performing the fix. This is necessary because macros can map the same - // spelling location to diff erent source locations, and we only want to fix - // the token once, before it is expanded by the macro. - SourceLocation FixLocation = Range.getBegin(); + // Get the spelling location for performing the fix. This is necessary because + // macros can map the same spelling location to diff erent source locations, + // and we only want to fix the token once, before it is expanded by the macro. + SourceLocation FixLocation = UsageRange.getBegin(); FixLocation = SourceMgr.getSpellingLoc(FixLocation); if (FixLocation.isInvalid()) - return; + return {NamingCheckFailures.end(), false}; + + auto EmplaceResult = NamingCheckFailures.try_emplace(FailureId); + NamingCheckFailure &Failure = EmplaceResult.first->second; // Try to insert the identifier location in the Usages map, and bail out if it // is already in there - RenamerClangTidyCheck::NamingCheckFailure &Failure = - NamingCheckFailures[Decl]; if (!Failure.RawUsageLocs.insert(FixLocation).second) - return; + return EmplaceResult; - if (!Failure.shouldFix()) - return; + if (Failure.FixStatus != RenamerClangTidyCheck::ShouldFixStatus::ShouldFix) + return EmplaceResult; if (SourceMgr.isWrittenInScratchSpace(FixLocation)) Failure.FixStatus = RenamerClangTidyCheck::ShouldFixStatus::InsideMacro; - if (!utils::rangeCanBeFixed(Range, &SourceMgr)) + if (!utils::rangeCanBeFixed(UsageRange, &SourceMgr)) Failure.FixStatus = RenamerClangTidyCheck::ShouldFixStatus::InsideMacro; + + return EmplaceResult; } -void RenamerClangTidyCheck::addUsage(const NamedDecl *Decl, SourceRange Range, +void RenamerClangTidyCheck::addUsage(const NamedDecl *Decl, + SourceRange UsageRange, const SourceManager &SourceMgr) { - // Don't keep track for non-identifier names. - auto *II = Decl->getIdentifier(); - if (!II) + if (hasNoName(Decl)) + return; + + // Ignore ClassTemplateSpecializationDecl which are creating duplicate + // replacements with CXXRecordDecl. + if (isa<ClassTemplateSpecializationDecl>(Decl)) return; - if (const auto *Method = dyn_cast<CXXMethodDecl>(Decl)) { - if (const CXXMethodDecl *Overridden = getOverrideMethod(Method)) - Decl = Overridden; - } - Decl = cast<NamedDecl>(Decl->getCanonicalDecl()); - return addUsage( - RenamerClangTidyCheck::NamingCheckId(Decl->getLocation(), II->getName()), - Range, SourceMgr); -} -void RenamerClangTidyCheck::checkNamedDecl(const NamedDecl *Decl, - const SourceManager &SourceMgr) { - std::optional<FailureInfo> MaybeFailure = getDeclFailureInfo(Decl, SourceMgr); + // We don't want to create a failure for every NamedDecl we find. Ideally + // there is just one NamedDecl in every group of "related" NamedDecls that + // becomes the failure. This NamedDecl and all of its related NamedDecls + // become usages. E.g. Since NamedDecls are Redeclarable, only the canonical + // NamedDecl becomes the failure and all redeclarations become usages. + const NamedDecl *FailureDecl = getFailureForNamedDecl(Decl); + + std::optional<FailureInfo> MaybeFailure = + getDeclFailureInfo(FailureDecl, SourceMgr); if (!MaybeFailure) return; - FailureInfo &Info = *MaybeFailure; - NamingCheckFailure &Failure = - NamingCheckFailures[NamingCheckId(Decl->getLocation(), Decl->getName())]; - SourceRange Range = - DeclarationNameInfo(Decl->getDeclName(), Decl->getLocation()) - .getSourceRange(); - - const IdentifierTable &Idents = Decl->getASTContext().Idents; - auto CheckNewIdentifier = Idents.find(Info.Fixup); + NamingCheckId FailureId(FailureDecl->getLocation(), FailureDecl->getName()); + + auto [FailureIter, NewFailure] = addUsage(FailureId, UsageRange, SourceMgr); + + if (FailureIter == NamingCheckFailures.end()) { + // Nothing to do if the usage wasn't accepted. + return; + } + if (!NewFailure) { + // FailureInfo has already been provided. + return; + } + + // Update the stored failure with info regarding the FailureDecl. + NamingCheckFailure &Failure = FailureIter->second; + Failure.Info = std::move(*MaybeFailure); + + // Don't overwritte the failure status if it was already set. + if (!Failure.shouldFix()) { + return; + } + const IdentifierTable &Idents = FailureDecl->getASTContext().Idents; + auto CheckNewIdentifier = Idents.find(Failure.Info.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; - } else if (!isValidAsciiIdentifier(Info.Fixup)) { + } else if (!isValidAsciiIdentifier(Failure.Info.Fixup)) { Failure.FixStatus = ShouldFixStatus::FixInvalidIdentifier; } - - Failure.Info = std::move(Info); - addUsage(Decl, Range, SourceMgr); } void RenamerClangTidyCheck::check(const MatchFinder::MatchResult &Result) { diff --git a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h index be5b6f0c7f7678..3d5721b789ac2e 100644 --- a/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h +++ b/clang-tools-extra/clang-tidy/utils/RenamerClangTidyCheck.h @@ -115,15 +115,9 @@ class RenamerClangTidyCheck : public ClangTidyCheck { void expandMacro(const Token &MacroNameTok, const MacroInfo *MI, const SourceManager &SourceMgr); - void addUsage(const RenamerClangTidyCheck::NamingCheckId &Decl, - SourceRange Range, const SourceManager &SourceMgr); - - /// Convenience method when the usage to be added is a NamedDecl. void addUsage(const NamedDecl *Decl, SourceRange Range, const SourceManager &SourceMgr); - void checkNamedDecl(const NamedDecl *Decl, const SourceManager &SourceMgr); - protected: /// Overridden by derived classes, returns information about if and how a Decl /// failed the check. A 'std::nullopt' result means the Decl did not fail the @@ -158,6 +152,14 @@ class RenamerClangTidyCheck : public ClangTidyCheck { const NamingCheckFailure &Failure) const = 0; private: + // Manage additions to the Failure/usage map + // + // return the result of NamingCheckFailures::try_emplace() if the usage was + // accepted. + std::pair<NamingCheckFailureMap::iterator, bool> + addUsage(const RenamerClangTidyCheck::NamingCheckId &FailureId, + SourceRange UsageRange, const SourceManager &SourceMgr); + NamingCheckFailureMap NamingCheckFailures; const bool AggressiveDependentMemberLookup; }; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
