dcoughlin added a comment.
I agree that it is weird that region store modifies the value it was asked to
bind and over all this seems like the right approach.
My big concern with this patch is that the logic that looks whether an lval is
being stored into a non-reference location and dereferences it seems overly
broad. I'm worried that this could paper over other issues by "fixing up"
mismatches that are really the result of some other bug. Is there a way to make
that logic more targeted? For example, if it only applies to initializing char
arrays with string literals, that is a much more precise check for when to load
from the lvalue.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:536
@@ -535,3 +535,3 @@
} else {
// We bound the temp obj region to the CXXConstructExpr. Now recover
// the lazy compound value when the variable is not a reference.
----------------
It seems like this comment ("We bound the temp obj region to the
CXXConstructorExpr...") is incomplete/misleading (and has been for a while)
because this code is executed when there is no constructor. Can it be updated
to something more helpful?
================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:665
@@ +664,3 @@
+ // type, as any expression. We need to read this type and make the
+ // necessary conversions.
+ if (const RecordType *RT =
----------------
I am surprised Sema wouldn't have already added an implicit cast with an lvalue
to rvalue conversion when needed. It seems in many cases it already does. Can
you be more explicit in your comments about why this is needed and under what
situations Sema doesn't handle this logic? Is initializing an array with a
string literal the only time this is needed? If so, can the check be more
targeted? I'll note that `InitListExpr` has an `isStringLiteralInit()` method
and that CodeGen has a special case for it.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:669
@@ +668,3 @@
+ // For structures, check if the respective field is a reference.
+ // FIXME: What if fields mismatch?
+ const RecordDecl *RD = RT->getDecl();
----------------
NoQ wrote:
> Whoops, was a note to myself, didn't mean to leave it here, sorry. Will
> actually have a look and update.
It is probably also worth looking into what it would take to support unions
here.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:673
@@ +672,3 @@
+ auto FieldI = RD->field_begin(), FieldE = RD->field_end();
+ for (auto InitI = IE->rbegin(), InitE = IE->rend();
+ InitI != InitE; ++InitI) {
----------------
If you are going to iterate through the initializer expressions in reverse, you
also have to iterate through the field decls in reverse. Also, I think it is
worth commenting why you iterate in reverse order here.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:679
@@ +678,3 @@
+ SVal V = state->getSVal(E, LCtx);
+ if (E->isLValue() && !V.isUnknownOrUndef() &&
+ !FieldI->getType()->isReferenceType())
----------------
This logic for checking with an expression is an lvalue and whether the type of
the storage location is reference and loading from the lvalue if not is
duplicated 3 times in this patch. Can you factor it out and give an meaningful
name?
================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:696
@@ +695,3 @@
+ V = state->getSVal(V.castAs<Loc>(), ET);
+ vals = getBasicVals().consVals(V, vals);
+ }
----------------
I think it would good to add a test with array fillers if we don't already have
them. For example:
```
int a[2000] = {1, 2, 3};
clang_analyzer_eval(a[0] == 1);
clang_analyzer_eval(a[1999] == 0);
```
================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:699
@@ +698,3 @@
+ } else {
+ // Vector values, complex numbers, etc: No lvalues to expect.
+ for (auto InitI = IE->rbegin(), InitE = IE->rend();
----------------
Should there be an assert here to make sure we have enumerated all the cases?
I'm worried about the 'etc'. How will we know if we have not considered a case?
https://reviews.llvm.org/D23963
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
