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 <[email protected]> wrote: >> <bump> >> >> On Wed, Dec 14, 2011 at 12:12 PM, David Blaikie <[email protected]> wrote: >>> On Sat, Sep 24, 2011 at 7:21 AM, Ted Kremenek <[email protected]> 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> _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
