Thanks for the fix. At some point in the refactor I removed all virtual methods and forgot to add the destructor again when they were reintroduced. The class is never really deleted from the base class, though. Is there a way to tell the compiler that instead? Like, by making the destructor protected but non-virtual?
On Thu, Sep 4, 2014 at 12:11 PM, David Blaikie <[email protected]> wrote: > I cleaned up the resulting clang -Wnon-virtual-dtor warnings with r217167 > (feel free to fix otherwise/let me know if that wasn't the right approach). > > > On Thu, Sep 4, 2014 at 7:13 AM, Samuel Benzaquen <[email protected]> > wrote: > >> Author: sbenza >> Date: Thu Sep 4 09:13:58 2014 >> New Revision: 217152 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=217152&view=rev >> Log: >> Refactor VariantMatcher::MatcherOps to reduce the amount of generated >> code. >> >> Summary: >> Refactor VariantMatcher::MatcherOps to reduce the amount of generated >> code. >> - Make some code type agnostic and move it to the cpp file. >> - Return a DynTypedMatcher instead of storing the object in MatcherOps. >> >> This change reduces the number of symbols generated in Registry.cpp by >> ~19%, the object byte size by ~17% and the compilation time (in >> non-release mode) by ~20%. >> >> Reviewers: klimek >> >> Subscribers: klimek, cfe-commits >> >> Differential Revision: http://reviews.llvm.org/D5124 >> >> Modified: >> cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h >> cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h >> cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp >> cfe/trunk/lib/ASTMatchers/Dynamic/VariantValue.cpp >> >> Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=217152&r1=217151&r2=217152&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h (original) >> +++ cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h Thu Sep 4 >> 09:13:58 2014 >> @@ -342,6 +342,17 @@ public: >> /// This version enables \c tryBind() on the \c DynTypedMatcher. >> template <typename T> inline DynTypedMatcher(const BindableMatcher<T> >> &M); >> >> + /// \brief Construct from a variadic function. >> + typedef bool (*VariadicOperatorFunction)( >> + const ast_type_traits::DynTypedNode DynNode, ASTMatchFinder >> *Finder, >> + BoundNodesTreeBuilder *Builder, ArrayRef<DynTypedMatcher> >> InnerMatchers); >> + static DynTypedMatcher >> + constructVariadic(VariadicOperatorFunction Func, >> + ArrayRef<DynTypedMatcher> InnerMatchers) { >> + assert(InnerMatchers.size() > 0 && "Array must not be empty."); >> + return DynTypedMatcher(new VariadicStorage(Func, InnerMatchers)); >> + } >> + >> /// \brief Returns true if the matcher matches the given \c DynNode. >> bool matches(const ast_type_traits::DynTypedNode DynNode, >> ASTMatchFinder *Finder, BoundNodesTreeBuilder *Builder) >> const { >> @@ -372,9 +383,9 @@ public: >> /// This method verifies that the underlying matcher in \c Other can >> process >> /// nodes of types T. >> template <typename T> bool canConvertTo() const { >> - return getSupportedKind().isBaseOf( >> - ast_type_traits::ASTNodeKind::getFromNodeKind<T>()); >> + return >> canConvertTo(ast_type_traits::ASTNodeKind::getFromNodeKind<T>()); >> } >> + bool canConvertTo(ast_type_traits::ASTNodeKind To) const; >> >> /// \brief Construct a \c Matcher<T> interface around the dynamic >> matcher. >> /// >> @@ -416,9 +427,35 @@ private: >> const uint64_t ID; >> }; >> >> + class VariadicStorage : public MatcherStorage { >> + public: >> + VariadicStorage(VariadicOperatorFunction Func, >> + ArrayRef<DynTypedMatcher> InnerMatchers) >> + : MatcherStorage(InnerMatchers[0].getSupportedKind(), >> + reinterpret_cast<uint64_t>(this)), >> + Func(Func), InnerMatchers(InnerMatchers) {} >> + >> + bool matches(const ast_type_traits::DynTypedNode DynNode, >> + ASTMatchFinder *Finder, >> + BoundNodesTreeBuilder *Builder) const override { >> + return Func(DynNode, Finder, Builder, InnerMatchers); >> + } >> + >> + llvm::Optional<DynTypedMatcher> tryBind(StringRef ID) const override >> { >> + return llvm::None; >> + } >> + >> + private: >> + VariadicOperatorFunction Func; >> + std::vector<DynTypedMatcher> InnerMatchers; >> + }; >> + >> /// \brief Typed implementation of \c MatcherStorage. >> template <typename T> class TypedMatcherStorage; >> >> + /// \brief Internal constructor for \c constructVariadic. >> + DynTypedMatcher(MatcherStorage *Storage) : Storage(Storage) {} >> + >> IntrusiveRefCntPtr<const MatcherStorage> Storage; >> }; >> >> @@ -460,16 +497,8 @@ inline DynTypedMatcher::DynTypedMatcher( >> >> /// \brief Specialization of the conversion functions for QualType. >> /// >> -/// These specializations provide the Matcher<Type>->Matcher<QualType> >> +/// This specialization provides the Matcher<Type>->Matcher<QualType> >> /// conversion that the static API does. >> -template <> inline bool DynTypedMatcher::canConvertTo<QualType>() const { >> - const ast_type_traits::ASTNodeKind SourceKind = getSupportedKind(); >> - return SourceKind.isSame( >> - ast_type_traits::ASTNodeKind::getFromNodeKind<Type>()) || >> - SourceKind.isSame( >> - ast_type_traits::ASTNodeKind::getFromNodeKind<QualType>()); >> -} >> - >> template <> >> inline Matcher<QualType> DynTypedMatcher::convertTo<QualType>() const { >> assert(canConvertTo<QualType>()); >> >> Modified: cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h?rev=217152&r1=217151&r2=217152&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h (original) >> +++ cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h Thu Sep 4 >> 09:13:58 2014 >> @@ -93,13 +93,25 @@ class VariantMatcher { >> /// \brief Methods that depend on T from >> hasTypedMatcher/getTypedMatcher. >> class MatcherOps { >> public: >> - virtual ~MatcherOps(); >> - virtual bool canConstructFrom(const DynTypedMatcher &Matcher, >> - bool &IsExactMatch) const = 0; >> - virtual void constructFrom(const DynTypedMatcher &Matcher) = 0; >> - virtual void constructVariadicOperator( >> + MatcherOps(ast_type_traits::ASTNodeKind NodeKind) : >> NodeKind(NodeKind) {} >> + >> + bool canConstructFrom(const DynTypedMatcher &Matcher, >> + bool &IsExactMatch) const; >> + >> + /// \brief Convert \p Matcher the destination type and return it as >> a new >> + /// DynTypedMatcher. >> + virtual DynTypedMatcher >> + convertMatcher(const DynTypedMatcher &Matcher) const = 0; >> + >> + /// \brief Constructs a variadic typed matcher from \p InnerMatchers. >> + /// Will try to convert each inner matcher to the destination type >> and >> + /// return llvm::None if it fails to do so. >> + llvm::Optional<DynTypedMatcher> constructVariadicOperator( >> ast_matchers::internal::VariadicOperatorFunction Func, >> - ArrayRef<VariantMatcher> InnerMatchers) = 0; >> + ArrayRef<VariantMatcher> InnerMatchers) const; >> + >> + private: >> + ast_type_traits::ASTNodeKind NodeKind; >> }; >> >> /// \brief Payload interface to be specialized by each matcher type. >> @@ -110,7 +122,8 @@ class VariantMatcher { >> virtual ~Payload(); >> virtual llvm::Optional<DynTypedMatcher> getSingleMatcher() const = 0; >> virtual std::string getTypeAsString() const = 0; >> - virtual void makeTypedMatcher(MatcherOps &Ops) const = 0; >> + virtual llvm::Optional<DynTypedMatcher> >> + getTypedMatcher(const MatcherOps &Ops) const = 0; >> virtual bool isConvertibleTo(ast_type_traits::ASTNodeKind Kind, >> unsigned *Specificity) const = 0; >> }; >> @@ -158,9 +171,8 @@ public: >> /// that can, the result would be ambiguous and false is returned. >> template <class T> >> bool hasTypedMatcher() const { >> - TypedMatcherOps<T> Ops; >> - if (Value) Value->makeTypedMatcher(Ops); >> - return Ops.hasMatcher(); >> + if (!Value) return false; >> + return Value->getTypedMatcher(TypedMatcherOps<T>()).hasValue(); >> } >> >> /// \brief Determines if the contained matcher can be converted to \p >> Kind. >> @@ -182,10 +194,9 @@ public: >> /// Asserts that \c hasTypedMatcher<T>() is true. >> template <class T> >> ast_matchers::internal::Matcher<T> getTypedMatcher() const { >> - TypedMatcherOps<T> Ops; >> - Value->makeTypedMatcher(Ops); >> - assert(Ops.hasMatcher() && "hasTypedMatcher<T>() == false"); >> - return Ops.matcher(); >> + assert(hasTypedMatcher<T>() && "hasTypedMatcher<T>() == false"); >> + return Value->getTypedMatcher(TypedMatcherOps<T>()) >> + ->template convertTo<T>(); >> } >> >> /// \brief String representation of the type of the value. >> @@ -197,53 +208,27 @@ public: >> private: >> explicit VariantMatcher(Payload *Value) : Value(Value) {} >> >> + template <typename T> struct TypedMatcherOps; >> + >> class SinglePayload; >> class PolymorphicPayload; >> class VariadicOpPayload; >> >> - template <typename T> >> - class TypedMatcherOps : public MatcherOps { >> - public: >> - typedef ast_matchers::internal::Matcher<T> MatcherT; >> - >> - virtual bool canConstructFrom(const DynTypedMatcher &Matcher, >> - bool &IsExactMatch) const { >> - IsExactMatch = Matcher.getSupportedKind().isSame( >> - ast_type_traits::ASTNodeKind::getFromNodeKind<T>()); >> - return Matcher.canConvertTo<T>(); >> - } >> - >> - virtual void constructFrom(const DynTypedMatcher& Matcher) { >> - Out.reset(new MatcherT(Matcher.convertTo<T>())); >> - } >> - >> - virtual void constructVariadicOperator( >> - ast_matchers::internal::VariadicOperatorFunction Func, >> - ArrayRef<VariantMatcher> InnerMatchers) { >> - std::vector<DynTypedMatcher> DynMatchers; >> - for (size_t i = 0, e = InnerMatchers.size(); i != e; ++i) { >> - // Abort if any of the inner matchers can't be converted to >> - // Matcher<T>. >> - if (!InnerMatchers[i].hasTypedMatcher<T>()) { >> - return; >> - } >> - DynMatchers.push_back(InnerMatchers[i].getTypedMatcher<T>()); >> - } >> - Out.reset(new MatcherT( >> - new >> ast_matchers::internal::VariadicOperatorMatcherInterface<T>( >> - Func, DynMatchers))); >> - } >> - >> - bool hasMatcher() const { return Out.get() != nullptr; } >> - const MatcherT &matcher() const { return *Out; } >> - >> - private: >> - std::unique_ptr<MatcherT> Out; >> - }; >> - >> IntrusiveRefCntPtr<const Payload> Value; >> }; >> >> +template <typename T> >> +struct VariantMatcher::TypedMatcherOps : VariantMatcher::MatcherOps { >> + TypedMatcherOps() >> + : MatcherOps(ast_type_traits::ASTNodeKind::getFromNodeKind<T>()) {} >> + typedef ast_matchers::internal::Matcher<T> MatcherT; >> + >> + DynTypedMatcher >> + convertMatcher(const DynTypedMatcher &Matcher) const override { >> + return DynTypedMatcher(Matcher.convertTo<T>()); >> + } >> +}; >> + >> /// \brief Variant value class. >> /// >> /// Basically, a tagged union with value type semantics. >> >> Modified: cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp?rev=217152&r1=217151&r2=217152&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp (original) >> +++ cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp Thu Sep 4 09:13:58 >> 2014 >> @@ -26,6 +26,17 @@ void BoundNodesTreeBuilder::visitMatches >> } >> } >> >> +bool DynTypedMatcher::canConvertTo(ast_type_traits::ASTNodeKind To) >> const { >> + const auto From = getSupportedKind(); >> + auto QualKind = >> ast_type_traits::ASTNodeKind::getFromNodeKind<QualType>(); >> + auto TypeKind = ast_type_traits::ASTNodeKind::getFromNodeKind<Type>(); >> + /// Mimic the implicit conversions of Matcher<>. >> + /// - From Matcher<Type> to Matcher<QualType> >> + if (From.isSame(TypeKind) && To.isSame(QualKind)) return true; >> + /// - From Matcher<Base> to Matcher<Derived> >> + return From.isBaseOf(To); >> +} >> + >> DynTypedMatcher::MatcherStorage::~MatcherStorage() {} >> >> void BoundNodesTreeBuilder::addMatch(const BoundNodesTreeBuilder &Other) >> { >> >> Modified: cfe/trunk/lib/ASTMatchers/Dynamic/VariantValue.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/Dynamic/VariantValue.cpp?rev=217152&r1=217151&r2=217152&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/ASTMatchers/Dynamic/VariantValue.cpp (original) >> +++ cfe/trunk/lib/ASTMatchers/Dynamic/VariantValue.cpp Thu Sep 4 >> 09:13:58 2014 >> @@ -49,7 +49,32 @@ bool ArgKind::isConvertibleTo(ArgKind To >> return true; >> } >> >> -VariantMatcher::MatcherOps::~MatcherOps() {} >> +bool >> +VariantMatcher::MatcherOps::canConstructFrom(const DynTypedMatcher >> &Matcher, >> + bool &IsExactMatch) const { >> + IsExactMatch = Matcher.getSupportedKind().isSame(NodeKind); >> + return Matcher.canConvertTo(NodeKind); >> +} >> + >> +llvm::Optional<DynTypedMatcher> >> +VariantMatcher::MatcherOps::constructVariadicOperator( >> + ast_matchers::internal::VariadicOperatorFunction Func, >> + ArrayRef<VariantMatcher> InnerMatchers) const { >> + std::vector<DynTypedMatcher> DynMatchers; >> + for (const auto &InnerMatcher : InnerMatchers) { >> + // Abort if any of the inner matchers can't be converted to >> + // Matcher<T>. >> + if (!InnerMatcher.Value) >> + return llvm::None; >> + llvm::Optional<DynTypedMatcher> Inner = >> + InnerMatcher.Value->getTypedMatcher(*this); >> + if (!Inner) >> + return llvm::None; >> + DynMatchers.push_back(*Inner); >> + } >> + return DynTypedMatcher::constructVariadic(Func, DynMatchers); >> +} >> + >> VariantMatcher::Payload::~Payload() {} >> >> class VariantMatcher::SinglePayload : public VariantMatcher::Payload { >> @@ -65,10 +90,12 @@ public: >> .str(); >> } >> >> - void makeTypedMatcher(MatcherOps &Ops) const override { >> + llvm::Optional<DynTypedMatcher> >> + getTypedMatcher(const MatcherOps &Ops) const override { >> bool Ignore; >> if (Ops.canConstructFrom(Matcher, Ignore)) >> - Ops.constructFrom(Matcher); >> + return Matcher; >> + return llvm::None; >> } >> >> bool isConvertibleTo(ast_type_traits::ASTNodeKind Kind, >> @@ -104,7 +131,8 @@ public: >> return (Twine("Matcher<") + Inner + ">").str(); >> } >> >> - void makeTypedMatcher(MatcherOps &Ops) const override { >> + llvm::Optional<DynTypedMatcher> >> + getTypedMatcher(const MatcherOps &Ops) const override { >> bool FoundIsExact = false; >> const DynTypedMatcher *Found = nullptr; >> int NumFound = 0; >> @@ -124,7 +152,8 @@ public: >> } >> // We only succeed if we found exactly one, or if we found an exact >> match. >> if (Found && (FoundIsExact || NumFound == 1)) >> - Ops.constructFrom(*Found); >> + return *Found; >> + return llvm::None; >> } >> >> bool isConvertibleTo(ast_type_traits::ASTNodeKind Kind, >> @@ -165,8 +194,9 @@ public: >> return Inner; >> } >> >> - void makeTypedMatcher(MatcherOps &Ops) const override { >> - Ops.constructVariadicOperator(Func, Args); >> + llvm::Optional<DynTypedMatcher> >> + getTypedMatcher(const MatcherOps &Ops) const override { >> + return Ops.constructVariadicOperator(Func, Args); >> } >> >> bool isConvertibleTo(ast_type_traits::ASTNodeKind Kind, >> >> >> _______________________________________________ >> 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
