[PATCH] D40463: [analyzer] Fix false negative on post-increment of uninitialized variable.

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.

Thanks, this looks good to me!

Do you have commit access or do you need someone to commit it for you?



================
Comment at: lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp:64
+    if (const UnaryOperator *U = dyn_cast<UnaryOperator>(StoreE)) {
+      str = "The expression of the unary operator is an uninitialized value. "
+            "The computed value will also be garbage";
----------------
xazax.hun wrote:
> lebedev.ri wrote:
> > xazax.hun wrote:
> > > lebedev.ri wrote:
> > > > dcoughlin wrote:
> > > > > "Unary operator" is probably not something we should expect users to know about. How about just "The expression is an uninitialized value. The computed value will also be garbage."
> > > > > 
> > > > Yep, i did not like my wording either :)
> > > A value being undefined does nt mean uninitialized. E.g.: the result of a bad shift operation might be UndefVal as well.
> > > Aren't these diagnostics misleading in those cases? Or we do not care about those?
> > I think this is for @dcoughlin to answer.
> > But shift operation is a binary operator, so this diff should not change that.
> I was wondering about examples like:
> ```
> int x = 1 << -1;
> ++x;
> ```
> 
> In this particular case, it will not issue the misleading error message. The shift that results in an undefined value will generate an error node, so we will not warn for the pre increment.
> 
> But in general I am a bit uncomfortable about the assumption of this check: if a value is undefined the reason is that it is being uninitialized. 
> 
> Note that, of course this problem is not specific to this patch but a general question about the checker. 
It is a good point, but I think fixing it should wait until a later patch.


Repository:
  rC Clang

https://reviews.llvm.org/D40463