r217152 - Refactor VariantMatcher::MatcherOps to reduce the amount of generated code.
Samuel Benzaquen
sbenza at google.com
Thu Sep 4 09:35:52 PDT 2014
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 <dblaikie at gmail.com> 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 <sbenza at google.com>
> 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
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140904/c4333dae/attachment.html>
More information about the cfe-commits
mailing list