On Thu, Sep 4, 2014 at 12:40 PM, David Blaikie <[email protected]> wrote:
> > On Thu, Sep 4, 2014 at 9:35 AM, Samuel Benzaquen <[email protected]> > wrote: > >> 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? >> > > Sorry I didn't clarify - that's precisely what I did. The dtor in the base > class is non-virtual, and the class template that implements all the > derived classes is marked final - this suppresses clang's > -Wnon-virtual-dtor warning without needlessly adding a virtual dtor. > Lesson learned: Read the changes before writing about them =) > > >> >> 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
