Ping
On Mon, Dec 15, 2014 at 9:57 AM, Aaron Ballman <[email protected]> wrote: > 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
