[PATCH] Static analysis checker for catch handler inversion

[Aaron Ballman]

Ping

On Mon, Nov 17, 2014 at 4:15 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Fri, Nov 14, 2014 at 5:36 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> 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.
>
> 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