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
