[PATCH] Static analysis checker for catch handler inversion

[Richard Smith]

On Fri, Nov 14, 2014 at 7:55 AM, Aaron Ballman <aaron at aaronballman.com>
wrote:

> On Thu, Nov 13, 2014 at 10:06 PM, Richard Smith <richard at metafoo.co.uk>
> 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.


+      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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141114/e70f5754/attachment.html>