<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Apr 7, 2015 at 5:01 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Tue, Apr 7, 2015 at 6:02 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
> LGTM, thanks!<br>
><br>
><br>
> + if (RD->lookupInBases(CatchTypePublicBases::FindPublicBasesOfType,<br>
> &CTPB,<br>
> + Paths)) {<br>
> + const CXXCatchStmt *Problem = CTPB.getFoundHandler();<br>
> + if (!Paths.isAmbiguous(CTPB.getFoundHandlerType())) {<br>
><br>
> Is it possible that lookupInBases will find an ambiguous base (so we'll skip<br>
> issuing the warning) but there is another handler for an unambiguous base?<br>
> This seems like a really weird corner case (and a false negative for the<br>
> warning), so don't worry about it if ambiguity is hard to detect from<br>
> FindPublicBasesOfType.<br>
<br>
</span>Is this basically dr308? We do issue a false-positive in this case,<br>
unfortunately, but I can't see a particularly great way to handle it.<br></blockquote><div><br></div><div>I think it's an even more unlikely case than the one in DR308:</div><div><br></div><div>struct A {};</div><div>struct B : A {};</div><div>struct C : A {};</div><div>struct X {};</div><div>struct D : B, C, X {};</div><div><br></div><div>void f() {</div><div> try { throw D(); }</div><div> catch (A) {} // does not catch D</div><div> catch (X) {} // catches D</div><div> catch (D) {} // should warn here</div><div>}</div><div><br></div><div>I think we won't warn for 'catch (D)' because the handler found by CatchTypePublicBases will be the A handler, which is ambiguous -- we don't carry on and find the X handler.</div><div><br></div><div>(There's also the case which we discussed previously:</div><div><br></div><div>void g() {</div><div> try { throw D(); }</div><div> catch (A) {} // does not catch D</div><div> catch (B) {} // catches D, but we warn anyway</div><div>}</div><div><br></div><div>I don't think there's anything we can reasonably do to improve this, and I'm OK with a false positive in this case.)</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I'll land the patch as-is, but we can continue to improve. I'll file a<br>
bug about it once I'm certain all the bots are happy with the commit.<br></blockquote><div><br></div><div>SGTM</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks!<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
> On Tue, Apr 7, 2015 at 7:21 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
> wrote:<br>
>><br>
>> On Fri, Apr 3, 2015 at 8:44 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>> wrote:<br>
>> > Please give QualTypeExt a more specific name to indicate that it<br>
>> > represents<br>
>> > a caught type. CatchHandlerType perhaps?<br>
>><br>
>> Renamed to CatchHandlerType (also changed variables named QTE).<br>
>><br>
>> > Please give PublicBasesOfType a name that somehow relates to catch<br>
>> > handlers,<br>
>> > since it's specific to them. Also, perhaps make FindPublicBasesOfType a<br>
>> > static member of it, so that you don't need to provide so many public<br>
>> > accessors. Or maybe make PublicBasesOfType a struct, nest it inside<br>
>> > ActOnCXXTryBlock, and use a lambda as the callback function.<br>
>><br>
>> I changed the class name to CatchTypePublicBases, and changed<br>
>> FindPublicBasesOfType to be a static member. Also removed accessors<br>
>> that were no longer required.<br>
>><br>
>> > + for (const auto &P : Path) {<br>
>> ><br>
>> > Do you really need to walk the path? You should get called back once for<br>
>> > each base class, so this seems redundant.<br>
>><br>
>> You're correct, this was redundant. I changed it over to using the<br>
>> CXXBaseSpecifier directly, instead of the path elements.<br>
>><br>
>> Thanks!<br>
>><br>
>> ~Aaron<br>
>><br>
>> ><br>
>> > On Fri, Apr 3, 2015 at 5:30 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> Ping<br>
>> >><br>
>> >> On Sun, Mar 22, 2015 at 4:09 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> >> wrote:<br>
>> >> > Ping?<br>
>> >> ><br>
>> >> > On Thu, Feb 26, 2015 at 4:16 PM, Aaron Ballman<br>
>> >> > <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> >> > wrote:<br>
>> >> >> Updated patch attached. Comment below.<br>
>> >> >><br>
>> >> >> On Wed, Feb 18, 2015 at 4:54 PM, Richard Smith<br>
>> >> >> <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
>> >> >> wrote:<br>
>> >> >>> On Tue, Feb 10, 2015 at 12:40 PM, Aaron Ballman<br>
>> >> >>> <<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<br>
>> >> >>>> <<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<br>
>> >> >>>> > 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<br>
>> >> >>>> entirely<br>
>> >> >>>> workable. When the callback function passed to lookupInBases<br>
>> >> >>>> returns<br>
>> >> >>>> true, searching along that path stops. See CXXInheritance.cpp:251;<br>
>> >> >>>> if<br>
>> >> >>>> BaseMatches returns true, we record the path but never visit<br>
>> >> >>>> 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<br>
>> >> >>>> 'HandlerInversion::B<br>
>> >> >>>> &'}}<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<br>
>> >> >>>> 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.<br>
>> >> >>>> The<br>
>> >> >>>> callback is not called a subsequent time for B as a base of D, and<br>
>> >> >>>> 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,<br>
>> >> >>>> and<br>
>> >> >>>> call lookupInBases on the base class prior to returning true, but<br>
>> >> >>>> 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<br>
>> >> >>> path<br>
>> >> >>> 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<br>
>> >> >>>> suggesting<br>
>> >> >>>> below, but it's also causing problems with<br>
>> >> >>>> test\SemaCXX\unreachable-catch-clauses.cpp reporting the warning<br>
>> >> >>>> for<br>
>> >> >>>> the final handler twice, so it's not ready yet. Is this moving in<br>
>> >> >>>> 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<br>
>> >> >>>> > none<br>
>> >> >>>> > 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<br>
>> >> >>>> > 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<br>
>> >> >>>> > dr308::A<br>
>> >> >>>> > &'}}<br>
>> >> >>>> > // unreachable<br>
>> >> >>>> > - } catch (const B&) {<br>
>> >> >>>> > + } catch (const B&) { // expected-warning {{exception of<br>
>> >> >>>> > type<br>
>> >> >>>> > '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<br>
>> >> >>> '&'<br>
>> >> >>> from<br>
>> >> >>> this diagnostic; you can't get an exception object of reference<br>
>> >> >>> 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<br>
>> >> >> 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<br>
>> >> >>>> > of<br>
>> >> >>>> > the<br>
>> >> >>>> > warning is true, but the 'throw D()' will actually be caught by<br>
>> >> >>>> > 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<br>
>> >> >>>> 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<br>
>> >> >>>> > <<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<br>
>> >> >>>> >> <<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<br>
>> >> >>>> >> >>>> > better to<br>
>> >> >>>> >> >>>> > use<br>
>> >> >>>> >> >>>> > a<br>
>> >> >>>> >> >>>> > SmallVector here (and use a depth-first search rather<br>
>> >> >>>> >> >>>> > than<br>
>> >> >>>> >> >>>> > 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<br>
>> >> >>>> >> >>>> > while<br>
>> >> >>>> >> >>>> > modifying it<br>
>> >> >>>> >> >>>> > (even though it's actually correct in this case).<br>
>> >> >>>> >> >>>> > 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<br>
>> >> >>>> >> >>>> > 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<br>
>> >> >>>> >> >>>> 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<br>
>> >> >>>> >> >>> a<br>
>> >> >>>> >> >>> by<br>
>> >> >>>> >> >>> reference/by value mismatch.<br>
>> >> >>>> >> >><br>
>> >> >>>> >> >> That's a very good point. Thanks! I've added an additional<br>
>> >> >>>> >> >> test<br>
>> >> >>>> >> >> 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>
>> >> >>>> >> >>> +<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,<br>
>> >> >>>> >> >>> unambiguous<br>
>> >> >>>> >> >>> base<br>
>> >> >>>> >> >>> classes here. Rather than walk the base classes yourself,<br>
>> >> >>>> >> >>> you<br>
>> >> >>>> >> >>> could<br>
>> >> >>>> >> >>> use<br>
>> >> >>>> >> >>> CXXRecordDecl::lookupInBases to enumerate the base classes<br>
>> >> >>>> >> >>> and<br>
>> >> >>>> >> >>> paths,<br>
>> >> >>>> >> >>> and<br>
>> >> >>>> >> >>> then check the public unambiguous ones.<br>
>> >> >>>> >> >><br>
>> >> >>>> >> >> I've made this change in the attached patch. Additionally,<br>
>> >> >>>> >> >> this<br>
>> >> >>>> >> >> patch<br>
>> >> >>>> >> >> no longer pays attention to exception declarations that are<br>
>> >> >>>> >> >> 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>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>