[cfe-commits] Removing redundant default cases

[Ted Kremenek]

This looks great to me.

On Jan 20, 2012, at 5:51 PM, David Blaikie wrote:

> Bump with updated patch (fixes existing test cases that triggered this
> warning (well, suppressed the warning in those cases) & added one for
> the new warning, added a separate flag for the warning
> (-Wswitch-enum-redundant-default (suggestions welcome)) and grouped it
> under -Wswitch-enum). I've also used this warning to fix all the cases
> of this in LLVM & Clang (changes already checked in - except for some
> cases in tblgen-erated code and google test)
> 
> - David
> 
> On Tue, Jan 10, 2012 at 12:24 AM, David Blaikie <dblaikie at gmail.com> wrote:
>> <bump>
>> 
>> On Wed, Dec 14, 2011 at 12:12 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>> On Sat, Sep 24, 2011 at 7:21 AM, Ted Kremenek <kremenek at apple.com> wrote:
>>>> On Sep 23, 2011, at 9:33 PM, David Blaikie wrote:
>>>> 
>>>>> Hi Ted,
>>>>> 
>>>>> I tried -Wunreachable-code earlier today (Chandler had suggested it as a way to find/remove the dead code after llvm_unreachables I'd migrated yesterday) & it produced some very... interesting output. It did find the dead code after llvm_unreachable but it also found some other very strange cases. I was wondering what was up with that. Good to know it's WIP - any tips on the state of that? anywhere I'd be able to lend a hand?
>>>> 
>>>> Hi David,
>>>> 
>>>> The weirdest results I see from -Wunreachable-code tend to involve template code.  In templates, I've seen cases where a branch condition depends on a template parameter, and at template instantiation the branch condition may become a constant.  This can cause some code to (correctly) be flagged as unreachable for that particular instantiation of a template.  This of course is not an invariant for that template for *all* instantiations, so it's not a real issue.
>>>> 
>>>> The solution I had in mind to fix this problem is to enhance the CFG.  Instead of just pruning CFG edges, for templates we could record when an edge was pruned AND whether or not it was dependent on a template parameter.  Most analyses would  continue to see the CFG as they do now, but specific analyses (such as -Wunreachable-code) could do something a bit more clever and not treat such code as unreachable.
>>>> 
>>>>> 
>>>>> It wouldn't catch all the same cases ("case N: default: stuff" for example) but this solution isn't great either, it'll catch a variety of arcane cases that won't have trivial fixes.
>>>> 
>>>> Ah, interesting.  -Wunreachable-code looks for finding unreachable basic blocks, not looking at whether or not a label could never be used.  Those are different concepts.
>>>> 
>>>>> Chandler had mentioned the idea of this warning (well, something like it) yesterday but after I threw this together we were talking about it more & realized it'd be pretty tricky to get right with a nice multiline fixit that is very reliable (I get the impression that's what he's really interested in - really low (0?) false positive rate & accurate fixits - which would be awesome, but require a rather different fix)
>>>> 
>>>> I agree.  A warning like this in the compiler needs close to a zero false positive rate.
>>> 
>>> I'm just coming back to this (checking all my "loose ends" threads, or
>>> at least some of them) and I can't quite remember what the problem was
>>> here. Is there a case my warning would fire on that shouldn't be
>>> fixed? I don't think so. The real issue would be the fixit - removing
>>> the default label would always be valid, but that could introduce
>>> (probably fairly trivial) unreachable code. Adding a fixit to remove
>>> the whole block would be harder. Should we have the warning with no
>>> fixit at all?
>>> 
>>> Also, I'm still interested in checking in the mechanical fixes & so
>>> far as I know it's the intended/preferred way of writing switches in
>>> Clang - it's usually given as code review feedback, but as with
>>> everything that isn't verified, things slip through. So if someone
>>> wants to confirm/sign off on that I'll check those in & we can nut out
>>> the warning separately.
>>> 
>>> - David
> <excess_default_warning.diff>