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
