r217152 - Refactor VariantMatcher::MatcherOps to reduce the amount of generated code.
Samuel Benzaquen
sbenza at google.com
Thu Sep 4 10:03:24 PDT 2014
On Thu, Sep 4, 2014 at 12:40 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
> 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.
>
Lesson learned: Read the changes before writing about them =)
>
>
>>
>> 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/af498399/attachment.html>
More information about the cfe-commits
mailing list