Ping
On Thu, Dec 4, 2014 at 10: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
