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
CatchHandlerChecker_v6.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
