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'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. 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
