[cfe-dev] PR17558 - Question about Uninitialized Variables

[Michael Bao]

On Wed, Jan 8, 2014 at 7:51 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> The -Wuninitialized warning isn't ever going to have zero false negatives.
> Why is it important to support the non-idiomatic case of
>
>   (void)&x;
>
> rather than suggesting people use the normal idiom of
>
>   (void)x;
>
> ? I'm not fundamentally opposed to this, but it seems rather ad-hoc, and I'd
> like for us to have a good justification for this complexity.
>

I don't necessarily disagree with you here -- especially since it does
seem rather complicated for such a non-common way of doing things.

> That said, the patch is definitely going in the right direction, if we do
> want to handle this. I've not looked through VisitCastExprChildren, but I'm
> surprised by the complexity of it. How did you determine the set of things
> to check for there?
>

In VisitCastExprChildren, the main thing I'm doing is visiting all the
children of the cast expression to find the DeclRefExpr (so I can
handle the more general case of doing a cast and not have Clang mark
the variable as being initialized rather than ignored).

The CXXTypeId, CallExpr, CXXUuid, and SizeOf checks are there as they
are just the cases that I thought of that would potentially be using
the variable (and thus shouldn't be ignored). (I found these out
because some tests weren't passing). Actually on second thought, I
don't think sizeof should be in there.

Then finally the block expression, I had to look into its body rather
than its children (Also found this one out because I was no longer
passing a test).

After thinking about this code some more, I think there are two things
that can be done about this code:

1) If we really want to support this case, it might be best just to
have some code like:

> if (isa<UnaryOperator>(CSE->getSubExpr())) {
>   classify(CSE->getSubExpr()->getSubExpr(), Ignore);
>   return;
> }
> classify(CSE->getSubExpr(), Ignore);

This way we avoid the bulk of the complexity while still supporting this case.

2) If the more general case is to be supported, I think I should change the

>   if (isa<CXXTypeidExpr>(S) || isa<CallExpr>(S) || isa<CXXUuidofExpr>(S)
>       || isa<SizeOfPackExpr>(S)) {
>     classify(dyn_cast<Expr>(S), Ignore);
>     return;
>   }

to something that checks to see whether or not the DeclRefExpr has
already been classified. If it has, then we should behave like the
original code. If it hasn't, then ignore it. That probably would be
less hacky than this current solution.

Thoughts?