On 05/18/2017 12:55 PM, Prathamesh Kulkarni wrote: > Hi, > The attached patch tries to fix PR80806 by warning when a variable is > set using memset (and friends) but not used. I chose to warn in dse > pass since dse would detect if the variable passed as 1st argument is > a dead store. Does this approach look OK ? [ ... ] So I think the biggest question is whether or not the case like Martin's deserves a warning.
What we have is an object that is conditionally set but not used depending on inlining context. We've generally "allowed" inlining to expose new warnings in the sense that inlining may (for example) allow us to remove the addressibility property on an object -- which makes the object subject to the usual -Wuninitialized analysis. In fact, I think we've generally considered that a positive outcome because it's exposing bugs in subtle paths. I'm less sure that this case falls into that same category. What we're really talking about is warning for a partially dead store. Would we want a warning if rather than a memset this was a simple store? Is that the right guiding principle here? I hate to say it, but I wonder if this is another case where there likely won't be a clear consensus and we're going to end up with a two level warning system? For something like Martin's case what I really think we should do is sink the memset call into the conditional. In cases where "i" is not a constant, but actually has the value zero at runtime we win. -- So I've got no objections to the idea of using DSE to detect the dead store and potentially warn. My concern is are we in a case where that warning is going to annoy users and we end up needing a level of -Wunused-but-set. Jeff
