Thanks Ted! committed as r148640 (after I fixed the last lingering dead defaults in gtest so that Clang building LLVM/Clang will still be warning free - lest I incur the wrath of Chandler (who OK'd the change - I wasn't sure whether checking in downstream gtest changes was appropriate) or anyone else building with -Werror)
- David On Fri, Jan 20, 2012 at 9:11 PM, Ted Kremenek <[email protected]> wrote: > 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
