[PATCH] Static analysis checker for catch handler inversion

[Aaron Ballman]

Ping

On Mon, Dec 15, 2014 at 9:57 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> 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