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
