<div dir="ltr"><div>Please give QualTypeExt a more specific name to indicate that it represents a caught type. CatchHandlerType perhaps?</div><div><br></div>Please give PublicBasesOfType a name that somehow relates to catch handlers, since it's specific to them. Also, perhaps make FindPublicBasesOfType a static member of it, so that you don't need to provide so many public accessors. Or maybe make PublicBasesOfType a struct, nest it inside ActOnCXXTryBlock, and use a lambda as the callback function.<br><div class="gmail_extra"><br></div><div class="gmail_extra">+ for (const auto &P : Path) {<br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Do you really need to walk the path? You should get called back once for each base class, so this seems redundant.</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 3, 2015 at 5:30 PM, 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 Sun, Mar 22, 2015 at 4:09 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br>
> Ping?<br>
><br>
> On Thu, Feb 26, 2015 at 4:16 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br>
>> Updated patch attached. Comment below.<br>
>><br>
>> On Wed, Feb 18, 2015 at 4:54 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
>>> On Tue, Feb 10, 2015 at 12:40 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>>> wrote:<br>
>>>><br>
>>>> On Wed, Jan 14, 2015 at 10:31 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>>>> wrote:<br>
>>>> > + friend struct llvm::DenseMapInfo <QualTypeExt>;<br>
>>>> ><br>
>>>> > Extra space before <.<br>
>>>> ><br>
>>>> > +// It's OK to treat BaseSubobject as a POD type.<br>
>>>> ><br>
>>>> > Typo: BaseSubobject should be QualTypeExt.<br>
>>>><br>
>>>> I've fixed both those up locally.<br>
>>>><br>
>>>> ><br>
>>>> > + // Add direct base classes to the list of types to be checked.<br>
>>>> > [...]<br>
>>>> > + RD->lookupInBases(<br>
>>>> ><br>
>>>> > lookupInBases finds all bases, not just the direct ones.<br>
>>>><br>
>>>> I must be missing something, because that doesn't seem to be entirely<br>
>>>> workable. When the callback function passed to lookupInBases returns<br>
>>>> true, searching along that path stops. See CXXInheritance.cpp:251; if<br>
>>>> BaseMatches returns true, we record the path but never visit further<br>
>>>> bases.<br>
>>>><br>
>>>> That winds up being problematic for situations like:<br>
>>>><br>
>>>> struct B {};<br>
>>>> struct D : B {};<br>
>>>> struct D2 : D {};<br>
>>>><br>
>>>> void f6() {<br>
>>>> try {<br>
>>>> } catch (B &b) { // expected-note {{for type 'HandlerInversion::B &'}}<br>
>>>> } catch (D2 &d) { // expected-warning {{exception of type<br>
>>>> 'HandlerInversion::D2 &' will be caught by earlier handler}}<br>
>>>> }<br>
>>>> }<br>
>>>><br>
>>>> We process the handler for B& and add it to the map. Then we process<br>
>>>> the handler for D2&. When we call lookupInBases on D2, we find D<br>
>>>> first, and since it's access is public, the callback returns true. The<br>
>>>> callback is not called a subsequent time for B as a base of D, and so<br>
>>>> we wind up failing to warn in that case.<br>
>>>><br>
>>>> Changing that "else if" to be an "if" is the wrong thing to do as<br>
>>>> well, because then a bunch of regression tests fail. I can kind of<br>
>>>> hack the behavior I want by having my callback not be a lambda, and<br>
>>>> call lookupInBases on the base class prior to returning true, but it<br>
>>>> feels like I'm doing something wrong. Ideas?<br>
>>><br>
>>><br>
>>> In Frobble, I think you should return true if you find a public path to a<br>
>>> HandledType, not just if you find a public base class.<br>
>>><br>
>>>> I've attached a WIP that attempts to capture what you're suggesting<br>
>>>> below, but it's also causing problems with<br>
>>>> test\SemaCXX\unreachable-catch-clauses.cpp reporting the warning for<br>
>>>> the final handler twice, so it's not ready yet. Is this moving in the<br>
>>>> direction you were thinking (aside from obvious issues like the<br>
>>>> callback being named Frobble)?<br>
>>><br>
>>><br>
>>> Yes, thanks.<br>
>>><br>
>>>> > You should be able<br>
>>>> > to do something like this:<br>
>>>> ><br>
>>>> > 1) If the type is a class type, call lookupInBases, and check none of<br>
>>>> > the<br>
>>>> > public unambiguous bases are in the map.<br>
>>>> > 2) Add the type itself to the map and diagnose if it was already there.<br>
>>>> ><br>
>>>> > (and remove your explicit queue of types to process).<br>
>>>> ><br>
>>>> ><br>
>>>> > --- test/CXX/drs/dr3xx.cpp (revision 222171)<br>
>>>> > +++ test/CXX/drs/dr3xx.cpp (working copy)<br>
>>>> > @@ -170,9 +170,9 @@<br>
>>>> > void f() {<br>
>>>> > try {<br>
>>>> > throw D();<br>
>>>> > - } catch (const A&) {<br>
>>>> > + } catch (const A&) { // expected-note {{for type 'const dr308::A<br>
>>>> > &'}}<br>
>>>> > // unreachable<br>
>>>> > - } catch (const B&) {<br>
>>>> > + } catch (const B&) { // expected-warning {{exception of type 'const<br>
>>>> > dr308::B &' will be caught by earlier handler}}<br>
>>><br>
>>><br>
>>> Hmm, while we're here, it'd be good to strip off the 'const' and '&' from<br>
>>> this diagnostic; you can't get an exception object of reference type.<br>
>><br>
>> You can still have a handler of reference type that the exception<br>
>> object binds to (which is the type the user is most likely to notice<br>
>> since it's visibly spelled out). Also, since this diagnostic sometimes<br>
>> needs to print the pointer, I'm not certain it's worth the added<br>
>> complexity to strip in the case of references.<br>
>><br>
>> Thanks!<br>
>><br>
>> ~Aaron<br>
>><br>
>>><br>
>>>><br>
>>>> > // get here instead<br>
>>>> > }<br>
>>>> > }<br>
>>>> ><br>
>>>> > Yikes, the warning is sort of a false-positive here! (The text of the<br>
>>>> > warning is true, but the 'throw D()' will actually be caught by the<br>
>>>> > second<br>
>>>> > handler, because A is an ambiguous base of D).<br>
>>>><br>
>>>> Oye, that is basically a false positive, and I'm not certain there's<br>
>>>> much I could realistically do about it either. :-(<br>
>>>><br>
>>>> Thanks!<br>
>>>><br>
>>>> ~Aaron<br>
>>>><br>
>>>> ><br>
>>>> > On Thu, Dec 4, 2014 at 7:23 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>>>> > wrote:<br>
>>>> >><br>
>>>> >> Ping<br>
>>>> >><br>
>>>> >> On Tue, Nov 25, 2014 at 9:59 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>>>> >> wrote:<br>
>>>> >> > Ping<br>
>>>> >> ><br>
>>>> >> > On Mon, Nov 17, 2014 at 4:15 PM, Aaron Ballman<br>
>>>> >> > <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>>>> >> > wrote:<br>
>>>> >> >> On Fri, Nov 14, 2014 at 5:36 PM, Richard Smith<br>
>>>> >> >> <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>>>> >> >> wrote:<br>
>>>> >> >>> On Fri, Nov 14, 2014 at 7:55 AM, Aaron Ballman<br>
>>>> >> >>> <<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<br>
>>>> >> >>>> <<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<br>
>>>> >> >>>> > use<br>
>>>> >> >>>> > 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<br>
>>>> >> >>>> > modifying it<br>
>>>> >> >>>> > (even though it's actually correct in this case). Alternative<br>
>>>> >> >>>> > 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<br>
>>>> >> >>>> > 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<br>
>>>> >> >>>> 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<br>
>>>> >> >> 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>
>>>> >> >>> +<br>
>>>> >> >>> CurType.isReference()));<br>
>>>> >> >>><br>
>>>> >> >>> Per [except.handle]p1, you should only consider public, unambiguous<br>
>>>> >> >>> base<br>
>>>> >> >>> classes here. Rather than walk the base classes yourself, you could<br>
>>>> >> >>> use<br>
>>>> >> >>> CXXRecordDecl::lookupInBases to enumerate the base classes and<br>
>>>> >> >>> paths,<br>
>>>> >> >>> and<br>
>>>> >> >>> then check the public unambiguous ones.<br>
>>>> >> >><br>
>>>> >> >> I've made this change in the attached patch. Additionally, this<br>
>>>> >> >> 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>
>>>> ><br>
>>>> ><br>
>>><br>
>>><br>
</div></div></blockquote></div><br></div></div>