Yes, I am still very much interested in pushing this through, although I did
get a bit sidetracked by the lifetime extension issue. It appears that it is
not entirely clear whether a construct like this should extend the lifetime of
the temporary or not <http://llvm.org/bugs/show_bug.cgi?id=17003>. While I
agree that it makes sense to extend, not even the clang compiler does that
currently. Therefore, I would propose to make sure code like this doesn't crash
the analyzer (I have made D1513 for that purpose and I think it should go in
first (review, please? :) )), and then this patch could go in, without the
lifetime extension (at least we will be consistent with the compiler).
If it turns out later that we do want to extend the lifetime, then I think
the same technique that we use for normal temporaries should work - just branch
once more on the condition of the ?: operator. In fact, I think that if we
change the CFG construction, then the rest of the analyzer should "just work".
I see no fundamental difference between these two cases (running the destructor
after the full statement or the block expression) -- the liveness analysis
should make sure the value of the condition remains in the state and can be
queried later.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1362
@@ +1361,3 @@
+ // If the value is already available, we don't need to do anything.
+ if(Pred->getState()->getSVal(Condition,
Pred->getLocationContext()).isUnknownOrUndef()) {
+ // Resolve the condition in the presence of nested '||' and '&&'.
----------------
Jordan Rose wrote:
> This looks like it's from the other patch, doesn't it?
It is related to it, but I didn't want to put it there, as it is not necessary
if we don't have temporary destructors. Basically, this will only be false when
we run through the branches the second time. (Ok, it can be false also in other
cases, but in these cases the normal ResolveCondition process will return the
same value anyway).
The catch is that when you evaluate the branches the first time you always
branch on the leaf values. Only in the second run you can have compound logical
expressions like (a && b) if b doesn't contain temporaries.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:99-102
@@ -98,2 +98,6 @@
Ty = AT->getElementType();
- LValue = State->getLValue(Ty, SVB.makeZeroArrayIndex(), LValue);
+ // FIXME: The if test is just a temporary workaround, because
+ // ProcessTemporaryDtor sends us NULL regions. It will not be necessary
once
+ // we can properly process temporary destructors.
+ if (LValue.getAsRegion())
+ LValue = State->getLValue(Ty, SVB.makeZeroArrayIndex(), LValue);
----------------
Jordan Rose wrote:
> This could probably be put outside the loop, huh?
Yes, indeed it can.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp:810-814
@@ -809,1 +809,7 @@
+ // Temporary object destructor processing is currently broken, so we never
+ // inline them.
+ // FIXME: Remove this once temp destructors are working.
+ if ((*currBldrCtx->getBlock())[currStmtIdx].getAs<CFGTemporaryDtor>())
+ return false;
+
----------------
Jordan Rose wrote:
> Does it make sense to check the call kind first? These are all simple
> operations but they do require several dereferences.
Well... I wanted to make sure we don't attempt the inline under any
circumstances. E.g. if I put it lower, then we would still inline synthesized
destructor calls because of the check below (I didn't test that, but it
certainly seems so). It would be possible to redesign the code to run this
check only if isBodyAutosynthesized et al. return true but that would make the
code hairy and i'm not sure if it's worth the trouble.
http://llvm-reviews.chandlerc.com/D1259
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
