This looks really great. Let's start getting bits of this checked in!
I've marked a few pieces of this patch that I'd like to be checked in as
separate pieces (to avoid the big monolithic change here). In addition to
those, can you factor out the `CXXDefaultInitExpr` handling code into a
separate patch, too?
Additionally, now we've basically converged on the functional side, there's a
couple of coding standard issues that should be sorted out prior to commit.
Variable names should be capitalized (don't worry about pre-existing variables
in this file, it's currently a big mess with regard to coding style issues),
and you have a "* " in `ClassifyRefs::VisitMemberExpr`.
After getting the side-pieces committed, we can circle back to the bulk of
this patch. There's only really one substantive comment that I have there (and
I think it's something you said you're already working on): it's hard to tell
if `CXXThisExpr` is being handled appropriately in all cases, and I'd like for
it to be tracked with a `ClassifyRefs`-based approach too (so if a
`CXXThisExpr` escapes we can conservatively-correctly mark all fields as
initialized). I don't know what you think about this -- do you think the patch
is already handling this sufficiently reliably that it'd be OK to commit
without this, and fix it up later? I know you've tested this against a lot of
code already, but cases like passing `this` through an `ObjCMessageExpr` or a
`CXXConstructExpr` (which may not have come up in your testing) don't look like
they're handled here.
================
Comment at: lib/Analysis/UninitializedValues.cpp:391
@@ +390,3 @@
+ ? cast<DeclRefExpr>(E)->getDecl()
+ : isa<MemberExpr>(E) ? cast<MemberExpr>(E)->getMemberDecl() : 0;
+ if (!D || !isTracked(D))
----------------
Do you need the `isa` check here? This should only be called on `DeclRefExpr`
and `MemberExpr`, as far as I can tell, so we should assert if something else
gets passed in (or, ideally, not compile).
================
Comment at: lib/Analysis/UninitializedValues.cpp:413-437
@@ -361,12 +412,27 @@
E = E->IgnoreParens();
- if (const ConditionalOperator *CO = dyn_cast<ConditionalOperator>(E)) {
- const Expr *TrueExpr = CO->getTrueExpr();
+ if (const AbstractConditionalOperator *ACO =
+ dyn_cast<AbstractConditionalOperator>(E)) {
+ if (isa<BinaryConditionalOperator>(ACO))
+ classify(cast<BinaryConditionalOperator>(ACO)->getCommon(), Use);
+ const Expr *TrueExpr = ACO->getTrueExpr();
if (!isa<OpaqueValueExpr>(TrueExpr))
classify(TrueExpr, C);
- classify(CO->getFalseExpr(), C);
+ classify(ACO->getFalseExpr(), C);
return;
}
- FindVarResult Var = findVar(E, DC);
- if (const DeclRefExpr *DRE = Var.getDeclRefExpr())
- Classification[DRE] = std::max(Classification[DRE], C);
+ if (const BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
+ switch (BO->getOpcode()) {
+ case BO_PtrMemD:
+ case BO_PtrMemI:
+ classify(BO->getLHS(), C);
+ return;
+ case BO_Comma:
+ classify(BO->getLHS(), Ignore);
+ classify(BO->getRHS(), C);
+ return;
+ default:
+ return;
+ }
+ }
+
----------------
The improvements here are great, but they're orthogonal to handling class
members. Please factor them out into a separate patch.
================
Comment at: lib/Analysis/UninitializedValues.cpp:393-394
@@ -392,4 +461,2 @@
classify(BO->getLHS(), Use);
- else if (BO->getOpcode() == BO_Assign)
- classify(BO->getLHS(), Ignore);
}
----------------
I'm concerned about this removal. The old model was:
* Visiting the `DeclRefExpr` on the LHS of an assignment does not initialize
the variable
* Visiting the assignment expression itself initializes the variable on its
LHS
The new model seems to be:
* Visiting the `DeclRefExpr` or `MemberExpr` on the LHS of an assignment
initializes the variable or field
* Visiting the assignment expression itself *also* initializes the variable
I think this may be subtly incorrect. If we have:
int x;
x = x + 1;
... and the CFG happens to emit the LHS and then the RHS, we won't find the
uninitialized use, because we'll think that `x` is already initialized.
================
Comment at: lib/Analysis/UninitializedValues.cpp:476-480
@@ -404,5 +475,7 @@
void ClassifyRefs::VisitCallExpr(CallExpr *CE) {
// If a value is passed by const reference to a function, we should not
assume
// that it is initialized by the call, and we conservatively do not assume
// that it is used.
+ // If the address of a value is passed to a function, we treat the value as
+ // initialized.
for (CallExpr::arg_iterator I = CE->arg_begin(), E = CE->arg_end();
----------------
This comment could describe what's going on here better. Usually if an address
is taken, we treat the value as initialized; the thing we should call out here
is that the value is *not* treated as initialized if passed to a const pointer
parameter. Maybe just change the first comment to "If a value is passed by
const pointer or const reference to a function, ..."?
================
Comment at: lib/Analysis/UninitializedValues.cpp:495-500
@@ -412,1 +494,8 @@
+ }
+ if (CXXMemberCallExpr *MCE = dyn_cast<CXXMemberCallExpr>(CE)) {
+ if (const MemberExpr *ME =
+ dyn_cast<MemberExpr>(MCE->getImplicitObjectArgument())) {
+ classify(ME, Use);
+ }
+ }
}
----------------
What happens here:
int f();
struct S {
int a, b;
int init() { a = b = 0; return f(); }
};
struct T {
int k;
S s;
T() { s.init(); } // ok? warning?
T(int) : k(s.init()) {} // ok? warning?
};
I think both of these are fine: because `T::s` has trivial initialization, its
lifetime begins when storage for it is obtained (per **[basic.life]p1**).
================
Comment at: lib/Analysis/UninitializedValues.cpp:519-523
@@ +518,7 @@
+ case CK_FunctionToPointerDecay:
+ if (const MemberExpr *ME = dyn_cast<MemberExpr>(CE->getSubExpr())) {
+ if (isa<CXXMethodDecl>(ME->getMemberDecl()) &&
+ cast<CXXMethodDecl>(ME->getMemberDecl())->isStatic())
+ classify(ME->getBase(), Ignore);
+ }
+ break;
----------------
Is this necessary? The `VisitMemberExpr` check seems like it should do the
right thing here.
================
Comment at: lib/Analysis/UninitializedValues.cpp:541-544
@@ +540,6 @@
+ // If it's not a reference, it will be classified appropriately by a cast
+ if (LME == ME && LME->getMemberDecl()->getType()->isReferenceType())
+ classify(LME, Use);
+ else if (LME != ME)
+ classify(LME, isFieldOrNonStaticMethod(ME->getMemberDecl()) ? Use :
Ignore);
+}
----------------
This doesn't look quite right (though I think the issue is in
`findLeftmostMemberExpr`). Consider:
this->member.static_member.non_static_member
When we visit this, I think we'll mark 'member' as `Use`. Suggestion: teach
`findLeftmostMemberExpr` not to walk through `MemberExpr`s for static data
members.
================
Comment at: lib/Analysis/UninitializedValues.cpp:547-550
@@ -425,2 +546,6 @@
+
+void ClassifyRefs::VisitCXXConstructExpr(CXXConstructExpr *CE) {
+ if (CE->getConstructor()->isCopyOrMoveConstructor())
+ classify(CE->getArg(0), Use);
}
----------------
I think you should also handle the arguments the same way that `VisitCallExpr`
does.
struct A { A(const int&); };
struct B {
int x, y;
B() { A a(x); y = x; } // should warn on this
};
I think we won't warn here, because our usual handling for conservatively
marking the `x` in `a(x)` as Ignore won't fire.
(Even if this is a copy or move constructor, it could still have additional
arguments, so you should do this in that case too.)
That said, this is also orthogonal to handling of fields, and fixing this in a
separate patch would be preferred.
================
Comment at: lib/Analysis/UninitializedValues.cpp:487-493
@@ +486,9 @@
+ continue;
+ } else if (IsPointerToConst((*I)->getType())) {
+ const Expr *Ex = stripCasts(DC->getParentASTContext(), *I);
+ const UnaryOperator *UO = dyn_cast<UnaryOperator>(Ex);
+ if (UO && UO->getOpcode() == UO_AddrOf)
+ Ex = UO->getSubExpr();
+ classify(Ex, Ignore);
+ }
+ }
----------------
This is orthogonal to field handling; please break this out into a separate
patch.
================
Comment at: lib/Analysis/UninitializedValues.cpp:993
@@ +992,3 @@
+ else if (Optional<CFGInitializer> ci = I->getAs<CFGInitializer>()) {
+ tf.setReportConstructor(isInClassInitializer(ctor, ++initIdx));
+ FieldDecl *fd = ci->getInitializer()->getMember();
----------------
This counting approach seems rather complex to me, and I think we can get the
same effect with something much simpler: remove the 'ReportConstructor' code
entirely, and when you come to issue a diagnostic for an uninitialized use of a
field, check whether the source location of the use is within the source range
of the constructor. If not, emit the additional note pointing out which
constructor it was.
================
Comment at: lib/Sema/AnalysisBasedWarnings.cpp:1672
@@ -1651,1 +1671,3 @@
+ .setAlwaysAdd(Stmt::MemberExprClass)
+ .setAlwaysAdd(Stmt::CXXDefaultInitExprClass)
.setAlwaysAdd(Stmt::ImplicitCastExprClass)
----------------
I don't think you need this any more.
http://llvm-reviews.chandlerc.com/D2181
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
