On Tue, Apr 7, 2015 at 5:01 PM, Aaron Ballman <[email protected]> wrote:
> On Tue, Apr 7, 2015 at 6:02 PM, Richard Smith <[email protected]> > wrote: > > LGTM, thanks! > > > > > > + if (RD->lookupInBases(CatchTypePublicBases::FindPublicBasesOfType, > > &CTPB, > > + Paths)) { > > + const CXXCatchStmt *Problem = CTPB.getFoundHandler(); > > + if (!Paths.isAmbiguous(CTPB.getFoundHandlerType())) { > > > > Is it possible that lookupInBases will find an ambiguous base (so we'll > skip > > issuing the warning) but there is another handler for an unambiguous > base? > > This seems like a really weird corner case (and a false negative for the > > warning), so don't worry about it if ambiguity is hard to detect from > > FindPublicBasesOfType. > > Is this basically dr308? We do issue a false-positive in this case, > unfortunately, but I can't see a particularly great way to handle it. > I think it's an even more unlikely case than the one in DR308: struct A {}; struct B : A {}; struct C : A {}; struct X {}; struct D : B, C, X {}; void f() { try { throw D(); } catch (A) {} // does not catch D catch (X) {} // catches D catch (D) {} // should warn here } 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. (There's also the case which we discussed previously: void g() { try { throw D(); } catch (A) {} // does not catch D catch (B) {} // catches D, but we warn anyway } 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.) > I'll land the patch as-is, but we can continue to improve. I'll file a > bug about it once I'm certain all the bots are happy with the commit. > SGTM > Thanks! > > ~Aaron > > > > > On Tue, Apr 7, 2015 at 7:21 AM, Aaron Ballman <[email protected]> > > wrote: > >> > >> On Fri, Apr 3, 2015 at 8:44 PM, Richard Smith <[email protected]> > >> wrote: > >> > Please give QualTypeExt a more specific name to indicate that it > >> > represents > >> > a caught type. CatchHandlerType perhaps? > >> > >> Renamed to CatchHandlerType (also changed variables named QTE). > >> > >> > 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. > >> > >> I changed the class name to CatchTypePublicBases, and changed > >> FindPublicBasesOfType to be a static member. Also removed accessors > >> that were no longer required. > >> > >> > + for (const auto &P : Path) { > >> > > >> > Do you really need to walk the path? You should get called back once > for > >> > each base class, so this seems redundant. > >> > >> You're correct, this was redundant. I changed it over to using the > >> CXXBaseSpecifier directly, instead of the path elements. > >> > >> Thanks! > >> > >> ~Aaron > >> > >> > > >> > On Fri, Apr 3, 2015 at 5:30 PM, Aaron Ballman <[email protected] > > > >> > wrote: > >> >> > >> >> Ping > >> >> > >> >> On Sun, Mar 22, 2015 at 4:09 PM, Aaron Ballman < > [email protected]> > >> >> wrote: > >> >> > Ping? > >> >> > > >> >> > On Thu, Feb 26, 2015 at 4:16 PM, Aaron Ballman > >> >> > <[email protected]> > >> >> > wrote: > >> >> >> Updated patch attached. Comment below. > >> >> >> > >> >> >> On Wed, Feb 18, 2015 at 4:54 PM, Richard Smith > >> >> >> <[email protected]> > >> >> >> wrote: > >> >> >>> On Tue, Feb 10, 2015 at 12:40 PM, Aaron Ballman > >> >> >>> <[email protected]> > >> >> >>> wrote: > >> >> >>>> > >> >> >>>> On Wed, Jan 14, 2015 at 10:31 PM, Richard Smith > >> >> >>>> <[email protected]> > >> >> >>>> wrote: > >> >> >>>> > + friend struct llvm::DenseMapInfo <QualTypeExt>; > >> >> >>>> > > >> >> >>>> > Extra space before <. > >> >> >>>> > > >> >> >>>> > +// It's OK to treat BaseSubobject as a POD type. > >> >> >>>> > > >> >> >>>> > Typo: BaseSubobject should be QualTypeExt. > >> >> >>>> > >> >> >>>> I've fixed both those up locally. > >> >> >>>> > >> >> >>>> > > >> >> >>>> > + // Add direct base classes to the list of types to be > >> >> >>>> > checked. > >> >> >>>> > [...] > >> >> >>>> > + RD->lookupInBases( > >> >> >>>> > > >> >> >>>> > lookupInBases finds all bases, not just the direct ones. > >> >> >>>> > >> >> >>>> I must be missing something, because that doesn't seem to be > >> >> >>>> entirely > >> >> >>>> workable. When the callback function passed to lookupInBases > >> >> >>>> returns > >> >> >>>> true, searching along that path stops. See > CXXInheritance.cpp:251; > >> >> >>>> if > >> >> >>>> BaseMatches returns true, we record the path but never visit > >> >> >>>> further > >> >> >>>> bases. > >> >> >>>> > >> >> >>>> That winds up being problematic for situations like: > >> >> >>>> > >> >> >>>> struct B {}; > >> >> >>>> struct D : B {}; > >> >> >>>> struct D2 : D {}; > >> >> >>>> > >> >> >>>> void f6() { > >> >> >>>> try { > >> >> >>>> } catch (B &b) { // expected-note {{for type > >> >> >>>> 'HandlerInversion::B > >> >> >>>> &'}} > >> >> >>>> } catch (D2 &d) { // expected-warning {{exception of type > >> >> >>>> 'HandlerInversion::D2 &' will be caught by earlier handler}} > >> >> >>>> } > >> >> >>>> } > >> >> >>>> > >> >> >>>> We process the handler for B& and add it to the map. Then we > >> >> >>>> process > >> >> >>>> the handler for D2&. When we call lookupInBases on D2, we find D > >> >> >>>> first, and since it's access is public, the callback returns > true. > >> >> >>>> The > >> >> >>>> callback is not called a subsequent time for B as a base of D, > and > >> >> >>>> so > >> >> >>>> we wind up failing to warn in that case. > >> >> >>>> > >> >> >>>> Changing that "else if" to be an "if" is the wrong thing to do > as > >> >> >>>> well, because then a bunch of regression tests fail. I can kind > of > >> >> >>>> hack the behavior I want by having my callback not be a lambda, > >> >> >>>> and > >> >> >>>> call lookupInBases on the base class prior to returning true, > but > >> >> >>>> it > >> >> >>>> feels like I'm doing something wrong. Ideas? > >> >> >>> > >> >> >>> > >> >> >>> In Frobble, I think you should return true if you find a public > >> >> >>> path > >> >> >>> to a > >> >> >>> HandledType, not just if you find a public base class. > >> >> >>> > >> >> >>>> I've attached a WIP that attempts to capture what you're > >> >> >>>> suggesting > >> >> >>>> below, but it's also causing problems with > >> >> >>>> test\SemaCXX\unreachable-catch-clauses.cpp reporting the warning > >> >> >>>> for > >> >> >>>> the final handler twice, so it's not ready yet. Is this moving > in > >> >> >>>> the > >> >> >>>> direction you were thinking (aside from obvious issues like the > >> >> >>>> callback being named Frobble)? > >> >> >>> > >> >> >>> > >> >> >>> Yes, thanks. > >> >> >>> > >> >> >>>> > You should be able > >> >> >>>> > to do something like this: > >> >> >>>> > > >> >> >>>> > 1) If the type is a class type, call lookupInBases, and check > >> >> >>>> > none > >> >> >>>> > of > >> >> >>>> > the > >> >> >>>> > public unambiguous bases are in the map. > >> >> >>>> > 2) Add the type itself to the map and diagnose if it was > already > >> >> >>>> > there. > >> >> >>>> > > >> >> >>>> > (and remove your explicit queue of types to process). > >> >> >>>> > > >> >> >>>> > > >> >> >>>> > --- test/CXX/drs/dr3xx.cpp (revision 222171) > >> >> >>>> > +++ test/CXX/drs/dr3xx.cpp (working copy) > >> >> >>>> > @@ -170,9 +170,9 @@ > >> >> >>>> > void f() { > >> >> >>>> > try { > >> >> >>>> > throw D(); > >> >> >>>> > - } catch (const A&) { > >> >> >>>> > + } catch (const A&) { // expected-note {{for type 'const > >> >> >>>> > dr308::A > >> >> >>>> > &'}} > >> >> >>>> > // unreachable > >> >> >>>> > - } catch (const B&) { > >> >> >>>> > + } catch (const B&) { // expected-warning {{exception of > >> >> >>>> > type > >> >> >>>> > 'const > >> >> >>>> > dr308::B &' will be caught by earlier handler}} > >> >> >>> > >> >> >>> > >> >> >>> Hmm, while we're here, it'd be good to strip off the 'const' and > >> >> >>> '&' > >> >> >>> from > >> >> >>> this diagnostic; you can't get an exception object of reference > >> >> >>> type. > >> >> >> > >> >> >> You can still have a handler of reference type that the exception > >> >> >> object binds to (which is the type the user is most likely to > notice > >> >> >> since it's visibly spelled out). Also, since this diagnostic > >> >> >> sometimes > >> >> >> needs to print the pointer, I'm not certain it's worth the added > >> >> >> complexity to strip in the case of references. > >> >> >> > >> >> >> Thanks! > >> >> >> > >> >> >> ~Aaron > >> >> >> > >> >> >>> > >> >> >>>> > >> >> >>>> > // get here instead > >> >> >>>> > } > >> >> >>>> > } > >> >> >>>> > > >> >> >>>> > 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). > >> >> >>>> > >> >> >>>> Oye, that is basically a false positive, and I'm not certain > >> >> >>>> there's > >> >> >>>> much I could realistically do about it either. :-( > >> >> >>>> > >> >> >>>> Thanks! > >> >> >>>> > >> >> >>>> ~Aaron > >> >> >>>> > >> >> >>>> > > >> >> >>>> > On Thu, Dec 4, 2014 at 7:23 AM, Aaron Ballman > >> >> >>>> > <[email protected]> > >> >> >>>> > wrote: > >> >> >>>> >> > >> >> >>>> >> Ping > >> >> >>>> >> > >> >> >>>> >> On Tue, Nov 25, 2014 at 9:59 AM, Aaron Ballman > >> >> >>>> >> <[email protected]> > >> >> >>>> >> wrote: > >> >> >>>> >> > Ping > >> >> >>>> >> > > >> >> >>>> >> > On Mon, Nov 17, 2014 at 4:15 PM, Aaron Ballman > >> >> >>>> >> > <[email protected]> > >> >> >>>> >> > wrote: > >> >> >>>> >> >> On Fri, Nov 14, 2014 at 5:36 PM, Richard Smith > >> >> >>>> >> >> <[email protected]> > >> >> >>>> >> >> wrote: > >> >> >>>> >> >>> On Fri, Nov 14, 2014 at 7:55 AM, Aaron Ballman > >> >> >>>> >> >>> <[email protected]> > >> >> >>>> >> >>> wrote: > >> >> >>>> >> >>>> > >> >> >>>> >> >>>> On Thu, Nov 13, 2014 at 10:06 PM, Richard Smith > >> >> >>>> >> >>>> <[email protected]> > >> >> >>>> >> >>>> wrote: > >> >> >>>> >> >>>> > + std::list<QualType> BaseClassTypes; > >> >> >>>> >> >>>> > > >> >> >>>> >> >>>> > This will allocate memory for every node; it would be > >> >> >>>> >> >>>> > better to > >> >> >>>> >> >>>> > use > >> >> >>>> >> >>>> > a > >> >> >>>> >> >>>> > SmallVector here (and use a depth-first search rather > >> >> >>>> >> >>>> > than > >> >> >>>> >> >>>> > a > >> >> >>>> >> >>>> > breadth-first > >> >> >>>> >> >>>> > one so you're only modifying one end of your list). > >> >> >>>> >> >>>> > > >> >> >>>> >> >>>> > + for (auto &CurType : BaseClassTypes) { > >> >> >>>> >> >>>> > > >> >> >>>> >> >>>> > It's a bit scary to be iterating over your container > >> >> >>>> >> >>>> > while > >> >> >>>> >> >>>> > modifying it > >> >> >>>> >> >>>> > (even though it's actually correct in this case). > >> >> >>>> >> >>>> > Alternative > >> >> >>>> >> >>>> > idiom: > >> >> >>>> >> >>>> > > >> >> >>>> >> >>>> > SmallVector<QualType, 8> BaseClassTypes; > >> >> >>>> >> >>>> > BaseClassTypes.push_back(...); > >> >> >>>> >> >>>> > while (!BaseClassTypes.empty()) { > >> >> >>>> >> >>>> > QualType T = BaseClassTypes.pop_back_val(); > >> >> >>>> >> >>>> > // ... maybe push back some values. > >> >> >>>> >> >>>> > } > >> >> >>>> >> >>>> > >> >> >>>> >> >>>> I have a strong preference for your idiom. ;-) > >> >> >>>> >> >>>> > >> >> >>>> >> >>>> > > >> >> >>>> >> >>>> > + auto I = std::find_if( > >> >> >>>> >> >>>> > + HandledTypes.begin(), HandledTypes.end(), > >> >> >>>> >> >>>> > > >> >> >>>> >> >>>> > This is still quadratic-time; maybe use a DenseMap > with > >> >> >>>> >> >>>> > the > >> >> >>>> >> >>>> > canonical > >> >> >>>> >> >>>> > QualType plus a "was it a pointer" flag as a key? > >> >> >>>> >> >>>> > >> >> >>>> >> >>>> I've given this a shot in my latest patch. Thank you for > >> >> >>>> >> >>>> the > >> >> >>>> >> >>>> feedback! > >> >> >>>> >> >>> > >> >> >>>> >> >>> > >> >> >>>> >> >>> + unsigned IsReference : 1; > >> >> >>>> >> >>> > >> >> >>>> >> >>> I think we don't need / want an IsReference flag. > Consider: > >> >> >>>> >> >>> > >> >> >>>> >> >>> try { throw Derived(); } > >> >> >>>> >> >>> catch (Base) {} > >> >> >>>> >> >>> catch (Derived &d) {} > >> >> >>>> >> >>> > >> >> >>>> >> >>> The second catch handler is unreachable even though we > have > >> >> >>>> >> >>> a > >> >> >>>> >> >>> by > >> >> >>>> >> >>> reference/by value mismatch. > >> >> >>>> >> >> > >> >> >>>> >> >> That's a very good point. Thanks! I've added an additional > >> >> >>>> >> >> test > >> >> >>>> >> >> for > >> >> >>>> >> >> that case. > >> >> >>>> >> >> > >> >> >>>> >> >>> > >> >> >>>> >> >>> > >> >> >>>> >> >>> + QualType Underlying = CurType.underlying(); > >> >> >>>> >> >>> + if (auto *RD = Underlying->getAsCXXRecordDecl()) { > >> >> >>>> >> >>> + for (const auto &B : RD->bases()) > >> >> >>>> >> >>> + > >> >> >>>> >> >>> BaseClassTypes.push_back(QualTypeExt(B.getType(), > >> >> >>>> >> >>> CurType.isPointer(), > >> >> >>>> >> >>> + > >> >> >>>> >> >>> CurType.isReference())); > >> >> >>>> >> >>> > >> >> >>>> >> >>> 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. > >> >> >>>> >> >> > >> >> >>>> >> >> I've made this change in the attached patch. Additionally, > >> >> >>>> >> >> this > >> >> >>>> >> >> patch > >> >> >>>> >> >> no longer pays attention to exception declarations that > are > >> >> >>>> >> >> invalid, > >> >> >>>> >> >> such as can be had from using auto in a catch clause. > >> >> >>>> >> >> > >> >> >>>> >> >> Thanks! > >> >> >>>> >> >> > >> >> >>>> >> >> ~Aaron > >> >> >>>> > > >> >> >>>> > > >> >> >>> > >> >> >>> > >> > > >> > > > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
