r217152 - Refactor VariantMatcher::MatcherOps to reduce the amount of generated code.
David Blaikie
dblaikie at gmail.com
Thu Sep 4 09:40:05 PDT 2014
On Thu, Sep 4, 2014 at 9:35 AM, Samuel Benzaquen <sbenza at google.com> 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 <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/b8601874/attachment.html>
More information about the cfe-commits
mailing list