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. > > 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
