<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Nov 14, 2014 at 7:55 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>On Thu, Nov 13, 2014 at 10:06 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:<br>
> +    std::list<QualType> BaseClassTypes;<br>
><br>
> This will allocate memory for every node; it would be better to use a<br>
> SmallVector here (and use a depth-first search rather than a breadth-first<br>
> one so you're only modifying one end of your list).<br>
><br>
> +    for (auto &CurType : BaseClassTypes) {<br>
><br>
> It's a bit scary to be iterating over your container while modifying it<br>
> (even though it's actually correct in this case). Alternative idiom:<br>
><br>
> SmallVector<QualType, 8> BaseClassTypes;<br>
> BaseClassTypes.push_back(...);<br>
> while (!BaseClassTypes.empty()) {<br>
>   QualType T = BaseClassTypes.pop_back_val();<br>
>   // ... maybe push back some values.<br>
> }<br>
<br>
</span>I have a strong preference for your idiom. ;-)<br>
<span><br>
><br>
> +      auto I = std::find_if(<br>
> +          HandledTypes.begin(), HandledTypes.end(),<br>
><br>
> This is still quadratic-time; maybe use a DenseMap with the canonical<br>
> QualType plus a "was it a pointer" flag as a key?<br>
<br>
</span>I've given this a shot in my latest patch. Thank you for the feedback!</blockquote><div><br></div><div> +  unsigned IsReference : 1;</div><div><br></div><div>I think we don't need / want an IsReference flag. Consider:</div><div><br></div><div>  try { throw Derived(); }</div><div>  catch (Base) {}</div><div>  catch (Derived &d) {}</div><div><br></div><div>The second catch handler is unreachable even though we have a by reference/by value mismatch.</div><div><br></div><div><br></div><div>+      QualType Underlying = CurType.underlying();<br></div><div><div>+      if (auto *RD = Underlying->getAsCXXRecordDecl()) {</div></div><div><div>+        for (const auto &B : RD->bases())</div><div>+          BaseClassTypes.push_back(QualTypeExt(B.getType(), CurType.isPointer(),</div><div>+                                               CurType.isReference()));</div></div><div><br></div><div>Per [except.handle]p1, you should only consider public, unambiguous base classes here. Rather than walk the base classes yourself, you could use CXXRecordDecl::lookupInBases to enumerate the base classes and paths, and then check the public unambiguous ones.</div></div></div></div>