This is awesome, thanks Manuel! I can't wait to see this working :)
================
Comment at: lib/Analysis/CFG.cpp:3540
@@ -3582,60 +3539,3 @@
AbstractConditionalOperator *E, bool BindToTemporary) {
- // First add destructors for condition expression. Even if this will
- // unnecessarily create a block, this block will be used at least by the full
- // expression.
- autoCreateBlock();
- CFGBlock *ConfluenceBlock = VisitForTemporaryDtors(E->getCond());
- if (badCFG)
- return NULL;
- if (BinaryConditionalOperator *BCO
- = dyn_cast<BinaryConditionalOperator>(E)) {
- ConfluenceBlock = VisitForTemporaryDtors(BCO->getCommon());
- if (badCFG)
- return NULL;
- }
-
- // Try to add block with destructors for LHS expression.
- CFGBlock *LHSBlock = NULL;
- Succ = ConfluenceBlock;
- Block = NULL;
- LHSBlock = VisitForTemporaryDtors(E->getTrueExpr(), BindToTemporary);
- if (badCFG)
- return NULL;
-
- // Try to add block with destructors for RHS expression;
- Succ = ConfluenceBlock;
- Block = NULL;
- CFGBlock *RHSBlock = VisitForTemporaryDtors(E->getFalseExpr(),
- BindToTemporary);
- if (badCFG)
- return NULL;
-
- if (!RHSBlock && !LHSBlock) {
- // If neither LHS nor RHS expression had temporaries to destroy don't
create
- // more blocks.
- Block = ConfluenceBlock;
- return Block;
- }
-
- Block = createBlock(false);
- Block->setTerminator(CFGTerminator(E, true));
- assert(Block->getTerminator().isTemporaryDtorsBranch());
-
- // See if this is a known constant.
- const TryResult &KnownVal = tryEvaluateBool(E->getCond());
-
- if (LHSBlock) {
- addSuccessor(Block, LHSBlock, !KnownVal.isFalse());
- } else if (KnownVal.isFalse()) {
- addSuccessor(Block, NULL);
- } else {
- addSuccessor(Block, ConfluenceBlock);
- std::reverse(ConfluenceBlock->pred_begin(), ConfluenceBlock->pred_end());
- }
-
- if (!RHSBlock)
- RHSBlock = ConfluenceBlock;
-
- addSuccessor(Block, RHSBlock, !KnownVal.isTrue());
-
- return Block;
+ CFGBlock *CondBlock = VisitForTemporaryDtors(E->getCond());
+ CFGBlock *TrueBlock = VisitForTemporaryDtors(E->getTrueExpr());
----------------
Noob question: If we return FalseBlock below, do we need to visit CondBlock and
TrueBlock even if they aren't returned? If we do need to visit them, is the
order important? (Consider documenting this if this isn't super-obvious to
someone with more clang experience than me :) )
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:771
@@ +770,3 @@
+ !State->contains<InitializedTemporariesSet>(BTE));
+ State = State->add<InitializedTemporariesSet>(BTE);
+ StmtBldr.generateNode(BTE, Pred, State);
----------------
If includeTemporaryDtorsInCFG is not set, should we avoid this state
modification?
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1451
@@ -1433,1 +1450,3 @@
+ if (const CXXBindTemporaryExpr *BTE =
+ dyn_cast<CXXBindTemporaryExpr>(Condition)) {
----------------
I think it's worth pulling this out into a new processTemporary helper method
rather than adding it to processBranch: you're not reusing any of the logic in
processBranch in the temporary dtor case.
This would involve adding a new HandleTemporary helper (or similar) that you'd
call instead of HandleBranch, and that would call processTemporary
================
Comment at: test/Analysis/temporaries.cpp:269
@@ +268,3 @@
+ if (i < 3 && (i >= 2 || check(NoReturnDtor()))) {
+ // FIXME: Figure out why we don't run into this for i == 2.
+ // If we add if (i == 2 && (...)) we run into this case, though.
----------------
Don't we hit the NoReturnDtor when i == 0, which will prevent us from getting
to i==2? This seems correct as-is.
================
Comment at: test/Analysis/temporaries.cpp:275
@@ -228,2 +274,3 @@
+ }
void testBinaryOperatorShortcut(bool value) {
----------------
Can you add a test that cleans up multiple temporaries in one condition? Maybe
something like this (not sure if this compiles, we might need to add a
check2(obj1, obj2) function):
class Dtor {
~Dtor() {}
};
void testMultipleTemporaries(bool value) {
if (value) {
// ~Dtor should run before ~NoReturnDtor() because construction order is
guaranteed by comma operator
if (!value || check((NoReturnDtor(), Dtor())) || value) {
clang_analyzer_eval(true); // no warning, unreachable code
}
}
}
We could also make ~Dtor emit a warning which would only be triggered if Dtor
was constructed with a certain value (e.g. check(Dtor(/*warning=*/true),
NoReturnDtor()), then see if clang warns on that error that should never happen.
http://reviews.llvm.org/D3627
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
