Sent http://reviews.llvm.org/D5826 as a follow change.
On Thu, Oct 16, 2014 at 4:13 AM, Alexander Kornienko <[email protected]> wrote: > Sorry for not chiming in earlier. A few nits. > > On Wed, Oct 15, 2014 at 4:58 PM, Samuel Benzaquen <[email protected]> > wrote: > >> Author: sbenza >> Date: Wed Oct 15 09:58:46 2014 >> New Revision: 219792 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=219792&view=rev >> Log: >> Speed up hasName() matcher. >> >> Summary: >> Speed up hasName() matcher by skipping the expensive generation of the >> fully qualified name unless we need it. >> In the common case of matching an unqualified name, we don't need to >> generate the full name. We might not even need to copy any string at >> all. >> This change speeds up our clang-tidy benchmark by ~10% >> >> Reviewers: klimek >> >> Subscribers: klimek, cfe-commits >> >> Differential Revision: http://reviews.llvm.org/D5776 >> >> Modified: >> cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h >> cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h >> cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp >> >> Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h?rev=219792&r1=219791&r2=219792&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h (original) >> +++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h Wed Oct 15 09:58:46 >> 2014 >> @@ -1591,16 +1591,8 @@ inline internal::Matcher<Stmt> sizeOfExp >> /// \code >> /// namespace a { namespace b { class X; } } >> /// \endcode >> -AST_MATCHER_P(NamedDecl, hasName, std::string, Name) { >> - assert(!Name.empty()); >> - const std::string FullNameString = "::" + >> Node.getQualifiedNameAsString(); >> - const StringRef FullName = FullNameString; >> - const StringRef Pattern = Name; >> - if (Pattern.startswith("::")) { >> - return FullName == Pattern; >> - } else { >> - return FullName.endswith(("::" + Pattern).str()); >> - } >> +inline internal::Matcher<NamedDecl> hasName(StringRef Name) { >> + return internal::Matcher<NamedDecl>(new >> internal::HasNameMatcher(Name)); >> } >> >> /// \brief Matches NamedDecl nodes whose fully qualified names contain >> >> Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=219792&r1=219791&r2=219792&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h (original) >> +++ cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h Wed Oct 15 >> 09:58:46 2014 >> @@ -574,6 +574,33 @@ private: >> std::string Name; >> }; >> >> +/// \brief Matches named declarations with a specific name. >> +/// >> +/// See \c hasName() in ASTMatchers.h for details. >> +class HasNameMatcher : public SingleNodeMatcherInterface<NamedDecl> { >> + public: >> + explicit HasNameMatcher(StringRef Name); >> + >> + bool matchesNode(const NamedDecl &Node) const override; >> + >> + private: >> + /// \brief Unqualified match routine. >> + /// >> + /// It is much faster than the full match, but it only works for >> unqualified >> + /// matches. >> + bool matchesNodeUnqualified(const NamedDecl &Node) const; >> + >> + /// \brief Full match routine >> + /// >> + /// It generates the fully qualified name of the declaration (which is >> + /// expensive) before trying to match. >> + /// It is slower but simple and works on all cases. >> + bool matchesNodeFull(const NamedDecl &Node) const; >> + >> + const bool UseUnqualifiedMatch; >> + const std::string Name; >> +}; >> + >> /// \brief Matches declarations for QualType and CallExpr. >> /// >> /// Type argument DeclMatcherT is required by >> PolymorphicMatcherWithParam1 but >> >> Modified: cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp?rev=219792&r1=219791&r2=219792&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp (original) >> +++ cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp Wed Oct 15 09:58:46 >> 2014 >> @@ -13,6 +13,7 @@ >> >> #include "clang/ASTMatchers/ASTMatchers.h" >> #include "clang/ASTMatchers/ASTMatchersInternal.h" >> +#include "llvm/ADT/SmallString.h" >> #include "llvm/Support/ManagedStatic.h" >> >> namespace clang { >> @@ -221,6 +222,51 @@ bool AnyOfVariadicOperator(const ast_typ >> return false; >> } >> >> +HasNameMatcher::HasNameMatcher(StringRef NameRef) >> + : UseUnqualifiedMatch(NameRef.find("::") == NameRef.npos), >> Name(NameRef) { >> + assert(!Name.empty()); >> +} >> + >> +bool HasNameMatcher::matchesNodeUnqualified(const NamedDecl &Node) const >> { >> + assert(UseUnqualifiedMatch); >> + if (Node.getIdentifier()) { >> + // Simple name. >> + return Name == Node.getName(); >> + } else if (Node.getDeclName()) { >> > > http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return > > >> + // Name needs to be constructed. >> + llvm::SmallString<128> NodeName; >> + llvm::raw_svector_ostream OS(NodeName); >> + Node.printName(OS); >> + return Name == OS.str(); >> + } else { >> > > http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return > > >> + return false; >> + } >> +} >> + >> +bool HasNameMatcher::matchesNodeFull(const NamedDecl &Node) const { >> + llvm::SmallString<128> NodeName = StringRef("::"); >> + llvm::raw_svector_ostream OS(NodeName); >> + Node.printQualifiedName(OS); >> + const StringRef FullName = OS.str(); >> + const StringRef Pattern = Name; >> + if (Pattern.startswith("::")) { >> + return FullName == Pattern; >> + } else { >> > > http://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return > > >> + return FullName.endswith(("::" + Pattern).str()); >> > > If this shows on the profile, we can also avoid creating a temporary > string here: > > return FullName.endswith(Pattern) && > FullName.drop_back(Pattern.size()).endswith("::"); > > >> + } >> +} >> + >> +bool HasNameMatcher::matchesNode(const NamedDecl &Node) const { >> + // FIXME: There is still room for improvement, but it would require >> copying a >> + // lot of the logic from NamedDecl::printQualifiedName(). The >> benchmarks do >> + // not show like that extra complexity is needed right now. >> + if (UseUnqualifiedMatch) { >> + assert(matchesNodeUnqualified(Node) == matchesNodeFull(Node)); >> + return matchesNodeUnqualified(Node); >> + } >> + return matchesNodeFull(Node); >> +} >> + >> } // end namespace internal >> } // end namespace ast_matchers >> } // end namespace clang >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
