<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Sep 4, 2014 at 12:40 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank" class="cremed">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote"><div class="">On Thu, Sep 4, 2014 at 9:35 AM, Samuel Benzaquen <span dir="ltr"><<a href="mailto:sbenza@google.com" target="_blank" class="cremed">sbenza@google.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>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.</div>

<div>The class is never really deleted from the base class, though. Is there a way to tell the compiler that instead?</div>
<div>Like, by making the destructor protected but non-virtual?</div></div></blockquote><div><br></div></div><div>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.</div>
</div></div></div></blockquote><div><br></div><div>Lesson learned: Read the changes before writing about them =)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><div><br></div><div class="gmail_extra"><div class="gmail_quote">On Thu, Sep 4, 2014 at 12:11 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank" class="cremed">dblaikie@gmail.com</a>></span> wrote:<br>


<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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).</div>


<div class="gmail_extra"><br><br><div class="gmail_quote">
On Thu, Sep 4, 2014 at 7:13 AM, Samuel Benzaquen <span dir="ltr"><<a href="mailto:sbenza@google.com" target="_blank" class="cremed">sbenza@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">



Author: sbenza<br>
Date: Thu Sep  4 09:13:58 2014<br>
New Revision: 217152<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=217152&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project?rev=217152&view=rev</a><br>
Log:<br>
Refactor VariantMatcher::MatcherOps to reduce the amount of generated code.<br>
<br>
Summary:<br>
Refactor VariantMatcher::MatcherOps to reduce the amount of generated code.<br>
 - Make some code type agnostic and move it to the cpp file.<br>
 - Return a DynTypedMatcher instead of storing the object in MatcherOps.<br>
<br>
This change reduces the number of symbols generated in Registry.cpp by<br>
~19%, the object byte size by ~17% and the compilation time (in non-release mode) by ~20%.<br>
<br>
Reviewers: klimek<br>
<br>
Subscribers: klimek, cfe-commits<br>
<br>
Differential Revision: <a href="http://reviews.llvm.org/D5124" target="_blank" class="cremed">http://reviews.llvm.org/D5124</a><br>
<br>
Modified:<br>
    cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h<br>
    cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h<br>
    cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp<br>
    cfe/trunk/lib/ASTMatchers/Dynamic/VariantValue.cpp<br>
<br>
Modified: cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=217152&r1=217151&r2=217152&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h?rev=217152&r1=217151&r2=217152&view=diff</a><br>




==============================================================================<br>
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h (original)<br>
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h Thu Sep  4 09:13:58 2014<br>
@@ -342,6 +342,17 @@ public:<br>
   /// This version enables \c tryBind() on the \c DynTypedMatcher.<br>
   template <typename T> inline DynTypedMatcher(const BindableMatcher<T> &M);<br>
<br>
+  /// \brief Construct from a variadic function.<br>
+  typedef bool (*VariadicOperatorFunction)(<br>
+      const ast_type_traits::DynTypedNode DynNode, ASTMatchFinder *Finder,<br>
+      BoundNodesTreeBuilder *Builder, ArrayRef<DynTypedMatcher> InnerMatchers);<br>
+  static DynTypedMatcher<br>
+  constructVariadic(VariadicOperatorFunction Func,<br>
+                    ArrayRef<DynTypedMatcher> InnerMatchers) {<br>
+    assert(InnerMatchers.size() > 0 && "Array must not be empty.");<br>
+    return DynTypedMatcher(new VariadicStorage(Func, InnerMatchers));<br>
+  }<br>
+<br>
   /// \brief Returns true if the matcher matches the given \c DynNode.<br>
   bool matches(const ast_type_traits::DynTypedNode DynNode,<br>
                ASTMatchFinder *Finder, BoundNodesTreeBuilder *Builder) const {<br>
@@ -372,9 +383,9 @@ public:<br>
   /// This method verifies that the underlying matcher in \c Other can process<br>
   /// nodes of types T.<br>
   template <typename T> bool canConvertTo() const {<br>
-    return getSupportedKind().isBaseOf(<br>
-        ast_type_traits::ASTNodeKind::getFromNodeKind<T>());<br>
+    return canConvertTo(ast_type_traits::ASTNodeKind::getFromNodeKind<T>());<br>
   }<br>
+  bool canConvertTo(ast_type_traits::ASTNodeKind To) const;<br>
<br>
   /// \brief Construct a \c Matcher<T> interface around the dynamic matcher.<br>
   ///<br>
@@ -416,9 +427,35 @@ private:<br>
     const uint64_t ID;<br>
   };<br>
<br>
+  class VariadicStorage : public MatcherStorage {<br>
+  public:<br>
+    VariadicStorage(VariadicOperatorFunction Func,<br>
+                    ArrayRef<DynTypedMatcher> InnerMatchers)<br>
+        : MatcherStorage(InnerMatchers[0].getSupportedKind(),<br>
+                         reinterpret_cast<uint64_t>(this)),<br>
+          Func(Func), InnerMatchers(InnerMatchers) {}<br>
+<br>
+    bool matches(const ast_type_traits::DynTypedNode DynNode,<br>
+                 ASTMatchFinder *Finder,<br>
+                 BoundNodesTreeBuilder *Builder) const override {<br>
+      return Func(DynNode, Finder, Builder, InnerMatchers);<br>
+    }<br>
+<br>
+    llvm::Optional<DynTypedMatcher> tryBind(StringRef ID) const override {<br>
+      return llvm::None;<br>
+    }<br>
+<br>
+  private:<br>
+    VariadicOperatorFunction Func;<br>
+    std::vector<DynTypedMatcher> InnerMatchers;<br>
+  };<br>
+<br>
   /// \brief Typed implementation of \c MatcherStorage.<br>
   template <typename T> class TypedMatcherStorage;<br>
<br>
+  /// \brief Internal constructor for \c constructVariadic.<br>
+  DynTypedMatcher(MatcherStorage *Storage) : Storage(Storage) {}<br>
+<br>
   IntrusiveRefCntPtr<const MatcherStorage> Storage;<br>
 };<br>
<br>
@@ -460,16 +497,8 @@ inline DynTypedMatcher::DynTypedMatcher(<br>
<br>
 /// \brief Specialization of the conversion functions for QualType.<br>
 ///<br>
-/// These specializations provide the Matcher<Type>->Matcher<QualType><br>
+/// This specialization provides the Matcher<Type>->Matcher<QualType><br>
 /// conversion that the static API does.<br>
-template <> inline bool DynTypedMatcher::canConvertTo<QualType>() const {<br>
-  const ast_type_traits::ASTNodeKind SourceKind = getSupportedKind();<br>
-  return SourceKind.isSame(<br>
-             ast_type_traits::ASTNodeKind::getFromNodeKind<Type>()) ||<br>
-         SourceKind.isSame(<br>
-             ast_type_traits::ASTNodeKind::getFromNodeKind<QualType>());<br>
-}<br>
-<br>
 template <><br>
 inline Matcher<QualType> DynTypedMatcher::convertTo<QualType>() const {<br>
   assert(canConvertTo<QualType>());<br>
<br>
Modified: cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h?rev=217152&r1=217151&r2=217152&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h?rev=217152&r1=217151&r2=217152&view=diff</a><br>




==============================================================================<br>
--- cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h (original)<br>
+++ cfe/trunk/include/clang/ASTMatchers/Dynamic/VariantValue.h Thu Sep  4 09:13:58 2014<br>
@@ -93,13 +93,25 @@ class VariantMatcher {<br>
   /// \brief Methods that depend on T from hasTypedMatcher/getTypedMatcher.<br>
   class MatcherOps {<br>
   public:<br>
-    virtual ~MatcherOps();<br>
-    virtual bool canConstructFrom(const DynTypedMatcher &Matcher,<br>
-                                  bool &IsExactMatch) const = 0;<br>
-    virtual void constructFrom(const DynTypedMatcher &Matcher) = 0;<br>
-    virtual void constructVariadicOperator(<br>
+    MatcherOps(ast_type_traits::ASTNodeKind NodeKind) : NodeKind(NodeKind) {}<br>
+<br>
+    bool canConstructFrom(const DynTypedMatcher &Matcher,<br>
+                          bool &IsExactMatch) const;<br>
+<br>
+    /// \brief Convert \p Matcher the destination type and return it as a new<br>
+    /// DynTypedMatcher.<br>
+    virtual DynTypedMatcher<br>
+    convertMatcher(const DynTypedMatcher &Matcher) const = 0;<br>
+<br>
+    /// \brief Constructs a variadic typed matcher from \p InnerMatchers.<br>
+    /// Will try to convert each inner matcher to the destination type and<br>
+    /// return llvm::None if it fails to do so.<br>
+    llvm::Optional<DynTypedMatcher> constructVariadicOperator(<br>
         ast_matchers::internal::VariadicOperatorFunction Func,<br>
-        ArrayRef<VariantMatcher> InnerMatchers) = 0;<br>
+        ArrayRef<VariantMatcher> InnerMatchers) const;<br>
+<br>
+  private:<br>
+    ast_type_traits::ASTNodeKind NodeKind;<br>
   };<br>
<br>
   /// \brief Payload interface to be specialized by each matcher type.<br>
@@ -110,7 +122,8 @@ class VariantMatcher {<br>
     virtual ~Payload();<br>
     virtual llvm::Optional<DynTypedMatcher> getSingleMatcher() const = 0;<br>
     virtual std::string getTypeAsString() const = 0;<br>
-    virtual void makeTypedMatcher(MatcherOps &Ops) const = 0;<br>
+    virtual llvm::Optional<DynTypedMatcher><br>
+    getTypedMatcher(const MatcherOps &Ops) const = 0;<br>
     virtual bool isConvertibleTo(ast_type_traits::ASTNodeKind Kind,<br>
                                  unsigned *Specificity) const = 0;<br>
   };<br>
@@ -158,9 +171,8 @@ public:<br>
   /// that can, the result would be ambiguous and false is returned.<br>
   template <class T><br>
   bool hasTypedMatcher() const {<br>
-    TypedMatcherOps<T> Ops;<br>
-    if (Value) Value->makeTypedMatcher(Ops);<br>
-    return Ops.hasMatcher();<br>
+    if (!Value) return false;<br>
+    return Value->getTypedMatcher(TypedMatcherOps<T>()).hasValue();<br>
   }<br>
<br>
   /// \brief Determines if the contained matcher can be converted to \p Kind.<br>
@@ -182,10 +194,9 @@ public:<br>
   /// Asserts that \c hasTypedMatcher<T>() is true.<br>
   template <class T><br>
   ast_matchers::internal::Matcher<T> getTypedMatcher() const {<br>
-    TypedMatcherOps<T> Ops;<br>
-    Value->makeTypedMatcher(Ops);<br>
-    assert(Ops.hasMatcher() && "hasTypedMatcher<T>() == false");<br>
-    return Ops.matcher();<br>
+    assert(hasTypedMatcher<T>() && "hasTypedMatcher<T>() == false");<br>
+    return Value->getTypedMatcher(TypedMatcherOps<T>())<br>
+        ->template convertTo<T>();<br>
   }<br>
<br>
   /// \brief String representation of the type of the value.<br>
@@ -197,53 +208,27 @@ public:<br>
 private:<br>
   explicit VariantMatcher(Payload *Value) : Value(Value) {}<br>
<br>
+  template <typename T> struct TypedMatcherOps;<br>
+<br>
   class SinglePayload;<br>
   class PolymorphicPayload;<br>
   class VariadicOpPayload;<br>
<br>
-  template <typename T><br>
-  class TypedMatcherOps : public MatcherOps {<br>
-  public:<br>
-    typedef ast_matchers::internal::Matcher<T> MatcherT;<br>
-<br>
-    virtual bool canConstructFrom(const DynTypedMatcher &Matcher,<br>
-                                  bool &IsExactMatch) const {<br>
-      IsExactMatch = Matcher.getSupportedKind().isSame(<br>
-          ast_type_traits::ASTNodeKind::getFromNodeKind<T>());<br>
-      return Matcher.canConvertTo<T>();<br>
-    }<br>
-<br>
-    virtual void constructFrom(const DynTypedMatcher& Matcher) {<br>
-      Out.reset(new MatcherT(Matcher.convertTo<T>()));<br>
-    }<br>
-<br>
-    virtual void constructVariadicOperator(<br>
-        ast_matchers::internal::VariadicOperatorFunction Func,<br>
-        ArrayRef<VariantMatcher> InnerMatchers) {<br>
-      std::vector<DynTypedMatcher> DynMatchers;<br>
-      for (size_t i = 0, e = InnerMatchers.size(); i != e; ++i) {<br>
-        // Abort if any of the inner matchers can't be converted to<br>
-        // Matcher<T>.<br>
-        if (!InnerMatchers[i].hasTypedMatcher<T>()) {<br>
-          return;<br>
-        }<br>
-        DynMatchers.push_back(InnerMatchers[i].getTypedMatcher<T>());<br>
-      }<br>
-      Out.reset(new MatcherT(<br>
-          new ast_matchers::internal::VariadicOperatorMatcherInterface<T>(<br>
-              Func, DynMatchers)));<br>
-    }<br>
-<br>
-    bool hasMatcher() const { return Out.get() != nullptr; }<br>
-    const MatcherT &matcher() const { return *Out; }<br>
-<br>
-  private:<br>
-    std::unique_ptr<MatcherT> Out;<br>
-  };<br>
-<br>
   IntrusiveRefCntPtr<const Payload> Value;<br>
 };<br>
<br>
+template <typename T><br>
+struct VariantMatcher::TypedMatcherOps : VariantMatcher::MatcherOps {<br>
+  TypedMatcherOps()<br>
+      : MatcherOps(ast_type_traits::ASTNodeKind::getFromNodeKind<T>()) {}<br>
+  typedef ast_matchers::internal::Matcher<T> MatcherT;<br>
+<br>
+  DynTypedMatcher<br>
+  convertMatcher(const DynTypedMatcher &Matcher) const override {<br>
+    return DynTypedMatcher(Matcher.convertTo<T>());<br>
+  }<br>
+};<br>
+<br>
 /// \brief Variant value class.<br>
 ///<br>
 /// Basically, a tagged union with value type semantics.<br>
<br>
Modified: cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp?rev=217152&r1=217151&r2=217152&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp?rev=217152&r1=217151&r2=217152&view=diff</a><br>




==============================================================================<br>
--- cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp (original)<br>
+++ cfe/trunk/lib/ASTMatchers/ASTMatchersInternal.cpp Thu Sep  4 09:13:58 2014<br>
@@ -26,6 +26,17 @@ void BoundNodesTreeBuilder::visitMatches<br>
   }<br>
 }<br>
<br>
+bool DynTypedMatcher::canConvertTo(ast_type_traits::ASTNodeKind To) const {<br>
+  const auto From = getSupportedKind();<br>
+  auto QualKind = ast_type_traits::ASTNodeKind::getFromNodeKind<QualType>();<br>
+  auto TypeKind = ast_type_traits::ASTNodeKind::getFromNodeKind<Type>();<br>
+  /// Mimic the implicit conversions of Matcher<>.<br>
+  /// - From Matcher<Type> to Matcher<QualType><br>
+  if (From.isSame(TypeKind) && To.isSame(QualKind)) return true;<br>
+  /// - From Matcher<Base> to Matcher<Derived><br>
+  return From.isBaseOf(To);<br>
+}<br>
+<br>
 DynTypedMatcher::MatcherStorage::~MatcherStorage() {}<br>
<br>
 void BoundNodesTreeBuilder::addMatch(const BoundNodesTreeBuilder &Other) {<br>
<br>
Modified: cfe/trunk/lib/ASTMatchers/Dynamic/VariantValue.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/Dynamic/VariantValue.cpp?rev=217152&r1=217151&r2=217152&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/ASTMatchers/Dynamic/VariantValue.cpp?rev=217152&r1=217151&r2=217152&view=diff</a><br>




==============================================================================<br>
--- cfe/trunk/lib/ASTMatchers/Dynamic/VariantValue.cpp (original)<br>
+++ cfe/trunk/lib/ASTMatchers/Dynamic/VariantValue.cpp Thu Sep  4 09:13:58 2014<br>
@@ -49,7 +49,32 @@ bool ArgKind::isConvertibleTo(ArgKind To<br>
   return true;<br>
 }<br>
<br>
-VariantMatcher::MatcherOps::~MatcherOps() {}<br>
+bool<br>
+VariantMatcher::MatcherOps::canConstructFrom(const DynTypedMatcher &Matcher,<br>
+                                             bool &IsExactMatch) const {<br>
+  IsExactMatch = Matcher.getSupportedKind().isSame(NodeKind);<br>
+  return Matcher.canConvertTo(NodeKind);<br>
+}<br>
+<br>
+llvm::Optional<DynTypedMatcher><br>
+VariantMatcher::MatcherOps::constructVariadicOperator(<br>
+    ast_matchers::internal::VariadicOperatorFunction Func,<br>
+    ArrayRef<VariantMatcher> InnerMatchers) const {<br>
+  std::vector<DynTypedMatcher> DynMatchers;<br>
+  for (const auto &InnerMatcher : InnerMatchers) {<br>
+    // Abort if any of the inner matchers can't be converted to<br>
+    // Matcher<T>.<br>
+    if (!InnerMatcher.Value)<br>
+      return llvm::None;<br>
+    llvm::Optional<DynTypedMatcher> Inner =<br>
+        InnerMatcher.Value->getTypedMatcher(*this);<br>
+    if (!Inner)<br>
+      return llvm::None;<br>
+    DynMatchers.push_back(*Inner);<br>
+  }<br>
+  return DynTypedMatcher::constructVariadic(Func, DynMatchers);<br>
+}<br>
+<br>
 VariantMatcher::Payload::~Payload() {}<br>
<br>
 class VariantMatcher::SinglePayload : public VariantMatcher::Payload {<br>
@@ -65,10 +90,12 @@ public:<br>
         .str();<br>
   }<br>
<br>
-  void makeTypedMatcher(MatcherOps &Ops) const override {<br>
+  llvm::Optional<DynTypedMatcher><br>
+  getTypedMatcher(const MatcherOps &Ops) const override {<br>
     bool Ignore;<br>
     if (Ops.canConstructFrom(Matcher, Ignore))<br>
-      Ops.constructFrom(Matcher);<br>
+      return Matcher;<br>
+    return llvm::None;<br>
   }<br>
<br>
   bool isConvertibleTo(ast_type_traits::ASTNodeKind Kind,<br>
@@ -104,7 +131,8 @@ public:<br>
     return (Twine("Matcher<") + Inner + ">").str();<br>
   }<br>
<br>
-  void makeTypedMatcher(MatcherOps &Ops) const override {<br>
+  llvm::Optional<DynTypedMatcher><br>
+  getTypedMatcher(const MatcherOps &Ops) const override {<br>
     bool FoundIsExact = false;<br>
     const DynTypedMatcher *Found = nullptr;<br>
     int NumFound = 0;<br>
@@ -124,7 +152,8 @@ public:<br>
     }<br>
     // We only succeed if we found exactly one, or if we found an exact match.<br>
     if (Found && (FoundIsExact || NumFound == 1))<br>
-      Ops.constructFrom(*Found);<br>
+      return *Found;<br>
+    return llvm::None;<br>
   }<br>
<br>
   bool isConvertibleTo(ast_type_traits::ASTNodeKind Kind,<br>
@@ -165,8 +194,9 @@ public:<br>
     return Inner;<br>
   }<br>
<br>
-  void makeTypedMatcher(MatcherOps &Ops) const override {<br>
-    Ops.constructVariadicOperator(Func, Args);<br>
+  llvm::Optional<DynTypedMatcher><br>
+  getTypedMatcher(const MatcherOps &Ops) const override {<br>
+    return Ops.constructVariadicOperator(Func, Args);<br>
   }<br>
<br>
   bool isConvertibleTo(ast_type_traits::ASTNodeKind Kind,<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank" class="cremed">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>
</blockquote></div><br></div></div></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>