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