[PATCH] Static analysis checker for catch handler inversion

[Aaron Ballman]

Ping

On Thu, Dec 4, 2014 at 10:23 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> 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