[cfe-commits] Removing redundant default cases

[David Blaikie]

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