[PATCH] Static analysis checker for catch handler inversion

[Aaron Ballman]

Ping

On Tue, Nov 25, 2014 at 9:59 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> 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