<div dir="ltr"><div>+ friend struct llvm::DenseMapInfo <QualTypeExt>;<br></div><div><br></div><div>Extra space before <.</div><div><br></div>+// It's OK to treat BaseSubobject as a POD type.<br><div><br></div><div>Typo: BaseSubobject should be QualTypeExt.</div><div><br></div><div><div>+ // Add direct base classes to the list of types to be checked.</div><div>[...]</div><div>+ RD->lookupInBases(<br></div><div><br></div><div>lookupInBases finds all bases, not just the direct ones. You should be able to do something like this:</div></div><div><br></div><div>1) If the type is a class type, call lookupInBases, and check none of the public unambiguous bases are in the map.<br></div><div><div>2) Add the type itself to the map and diagnose if it was already there.</div></div><div><br></div><div>(and remove your explicit queue of types to process).</div><div><br></div><div><br></div><div><div>--- test/CXX/drs/dr3xx.cpp<span class="" style="white-space:pre"> </span>(revision 222171)</div><div>+++ test/CXX/drs/dr3xx.cpp<span class="" style="white-space:pre"> </span>(working copy)</div><div>@@ -170,9 +170,9 @@</div><div> void f() {</div><div> try {</div><div> throw D();</div><div>- } catch (const A&) {</div><div>+ } catch (const A&) { // expected-note {{for type 'const dr308::A &'}}</div><div> // unreachable</div><div>- } catch (const B&) {</div><div>+ } catch (const B&) { // expected-warning {{exception of type 'const dr308::B &' will be caught by earlier handler}}</div><div> // get here instead</div><div> }</div><div> }</div></div><div><br></div><div>Yikes, the warning is sort of a false-positive here! (The text of the warning is true, but the 'throw D()' will actually be caught by the second handler, because A is an ambiguous base of D).</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Dec 4, 2014 at 7:23 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">Ping<br>
<div class=""><div class="h5"><br>
On Tue, Nov 25, 2014 at 9:59 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br>
> Ping<br>
><br>
> On Mon, Nov 17, 2014 at 4:15 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br>
>> On Fri, Nov 14, 2014 at 5:36 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
>>> On Fri, Nov 14, 2014 at 7:55 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>>> wrote:<br>
>>>><br>
>>>> On Thu, Nov 13, 2014 at 10:06 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>>>> 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<br>
>>>> > 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>
>>>> I have a strong preference for your idiom. ;-)<br>
>>>><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>
>>>> I've given this a shot in my latest patch. Thank you for the feedback!<br>
>>><br>
>>><br>
>>> + unsigned IsReference : 1;<br>
>>><br>
>>> I think we don't need / want an IsReference flag. Consider:<br>
>>><br>
>>> try { throw Derived(); }<br>
>>> catch (Base) {}<br>
>>> catch (Derived &d) {}<br>
>>><br>
>>> The second catch handler is unreachable even though we have a by<br>
>>> reference/by value mismatch.<br>
>><br>
>> That's a very good point. Thanks! I've added an additional test for that case.<br>
>><br>
>>><br>
>>><br>
>>> + QualType Underlying = CurType.underlying();<br>
>>> + if (auto *RD = Underlying->getAsCXXRecordDecl()) {<br>
>>> + for (const auto &B : RD->bases())<br>
>>> + BaseClassTypes.push_back(QualTypeExt(B.getType(),<br>
>>> CurType.isPointer(),<br>
>>> + CurType.isReference()));<br>
>>><br>
>>> Per [except.handle]p1, you should only consider public, unambiguous base<br>
>>> classes here. Rather than walk the base classes yourself, you could use<br>
>>> CXXRecordDecl::lookupInBases to enumerate the base classes and paths, and<br>
>>> then check the public unambiguous ones.<br>
>><br>
>> I've made this change in the attached patch. Additionally, this patch<br>
>> no longer pays attention to exception declarations that are invalid,<br>
>> such as can be had from using auto in a catch clause.<br>
>><br>
>> Thanks!<br>
>><br>
>> ~Aaron<br>
</div></div></blockquote></div><br></div></div>