<div class="gmail_quote">On Sun, Mar 27, 2011 at 4:23 PM, Ted Kremenek <span dir="ltr"><<a href="mailto:kremenek@apple.com">kremenek@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div style="word-wrap:break-word">Hi Chandler, Richard,<div><br></div><div>I just noticed Chandler's commit.</div></div></blockquote><div><br></div><div>FYI, I saw your email on my commit, and was preparing to respond there, but I'll respond here so we can all follow the discussion.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div style="word-wrap:break-word"><div>I think this is a needed warning, but I'm a little concerned that the logic here is redundant.  Clang has a generic -Wuninitialized dataflow analysis, and it seems like cases like this should be caught there.  Doing an extra check seems like redundant work.  Was there a particular motivation to do the check this way?</div>
</div></blockquote><div><br></div><div>Two-fold:</div><div><br></div><div>The primary reason is that the data-flow analysis wasn't catching these. I specifically checked this prior to committing. I agree it's worth understanding why that is the case, but we were actively regressing as these bugs were being checked into our codebase. This is a particularly common typo for us, as we have several style rules that end up with 1-character edit distance between the intended initializer and initialized variable.</div>
<div><br></div><div>The secondary reason is that much like initializer lists, this is a case where we can relatively cheaply warn with extremely high accuracy without doing proper data-flow analyses. I wonder whether we should keep these warnings around and provide the option of disabling the dataflow-based warnings in the event those prove too expensive in some contexts... But not sure that's ever likely to be true. It's entirely possible we can make the dataflow we're already doing cheap enough to do in all cases.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div style="word-wrap:break-word"><div>I'm also mildly concerned that variable initialization is essentially a flow-sensitive property.  While contrived, the following case would falsely trigger warning from the logic added in r128376:</div>
<div><br></div><div>  int x = (x = 1, x);</div></div></blockquote><div><br></div><div>For reference, I consider this a feature rather than a bug. ;]</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div style="word-wrap:break-word"><div>While this is obviously contrived, my point is that reasoning about such cases doesn't require any explicit logic in the dataflow analysis; it just falls out.  While the dataflow analysis doesn't handle C++ initializers (yet) in constructors, when it does it will be able to handle control-flow dependencies between initializers as well.  My feeling is that since we already have a dataflow analysis for uninitialized variables, I'd rather focus on making that better than adding ad hoc checks for corner cases.</div>
<div><br></div><div>What do you guys think?</div></div></blockquote><div><br></div><div>I've no problem with this direction, but I'm also worried about how complex fixing the dataflow will be to get it to catch these cases. I'm happy to look into it, but I'd like to keep at least some checking of initializers given how common this bug has proven to be for us.</div>
</div>