I also thought again about the problem of where to reset the bound state of
the bind-temporary. The problem with the current solution is that it puts
pretty strong requirements on how the CFG looks: we need to guarantee that for
every constructed temporary, we will control-flow through the decision node
that branches to the destructor call. At a first glance this may seem benign,
but when I first thought about how to construct the CFG in that case, I
believed the "optimum" to be one where we don't have any decision blocks when
there is no control flow, and where we use the knowledge from where we are in a
logical operator chain to unconditionally flow into the next destructor block.
For example:
A() || B() || C() || D()
When we are in the destructor block for D(), we *know* that we have to call the
destructor of C(), thus we could unconditionally flow into C's destructor block
without first flowing into its decision block (like the current code does).
I'm not sure it makes sense to do that (as it's more complex), but on the other
hand I'm not sure it makes sense to preclude that with an implementation detail
of how we track the temp ctor calls.
================
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());
----------------
Alex McCarthy wrote:
> 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 :) )
Yes, the CFG blocks are hooked up even if they aren't returned. Actually, most
of the hooking up goes via the Block and Succ members, and afaiu the returned
value is mostly for convenience (and usually equals Block).
I'm not sure whether it makes sense to document that here, as it seems to be an
invariant used throughout the CFG builder.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:771
@@ +770,3 @@
+ !State->contains<InitializedTemporariesSet>(BTE));
+ State = State->add<InitializedTemporariesSet>(BTE);
+ StmtBldr.generateNode(BTE, Pred, State);
----------------
Alex McCarthy wrote:
> If includeTemporaryDtorsInCFG is not set, should we avoid this state
> modification?
That's a good question (for Jordan ;)
Another question is whether we still want to do the runCheckersForPre /
runCheckersForPost enchilada.
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:1451
@@ -1433,1 +1450,3 @@
+ if (const CXXBindTemporaryExpr *BTE =
+ dyn_cast<CXXBindTemporaryExpr>(Condition)) {
----------------
Alex McCarthy wrote:
> 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
I agree. I mainly haven't done that, as somewhere in that chain there is an
interface, and the method is abstract. I found only one use, so I assume
somebody else implements that interface outside of the clang codebase. I'd like
to hear what Jordan thinks on that issue, architecture-wise.
================
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.
----------------
Alex McCarthy wrote:
> Don't we hit the NoReturnDtor when i == 0, which will prevent us from getting
> to i==2? This seems correct as-is.
/me headdesks.
================
Comment at: test/Analysis/temporaries.cpp:275
@@ -228,2 +274,3 @@
+ }
void testBinaryOperatorShortcut(bool value) {
----------------
Alex McCarthy wrote:
> 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.
Added the test, and it works as expected. The problem is that I have no idea
how to verify the construction order, as (according to comments I've seen in
the code) the analyzer does not inline destructors (or at least temporary
destructors).
http://reviews.llvm.org/D3627
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
