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
