[cfe-commits] Patch: Add warning for variable initialization that references the value of itself

[Chandler Carruth]

On Sun, Mar 27, 2011 at 4:23 PM, Ted Kremenek <kremenek at apple.com> wrote:

> Hi Chandler, Richard,
>
> I just noticed Chandler's commit.
>

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.


> 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?
>

Two-fold:

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.

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.


> 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:
>
>   int x = (x = 1, x);
>

For reference, I consider this a feature rather than a bug. ;]


> 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.
>
> What do you guys think?
>

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110327/60739298/attachment.html>