[PATCH] Proposal on how to fix temporary dtors.

[Manuel Klimek]

On Wed, Jun 11, 2014 at 7:06 PM, Jordan Rose <jordan_rose at apple.com> wrote:

> Let's bring it home!
>

We still need to decide what to do with the FoldingSetNode patch - if you
prefer, I can just inline it into this patch into the .cc file, which would
still allow us to decide to put it into a more common place later?


> Thanks again for working on this. I'm excited to have it in trunk and
> hopefully soon to turn it on by default.
>
> ================
> Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:521-527
> @@ +520,9 @@
> +  }
> +  StmtNodeBuilder StmtBldr(PreVisit, Dst, *currBldrCtx);
> +  ProgramStateRef State = Pred->getState();
> +  assert(!State->contains<InitializedTemporariesSet>(
> +      std::make_pair(BTE, Pred->getStackFrame())));
> +  State = State->add<InitializedTemporariesSet>(
> +      std::make_pair(BTE, Pred->getStackFrame()));
> +  StmtBldr.generateNode(BTE, Pred, State);
> +}
> ----------------
> Manuel Klimek wrote:
> > Jordan Rose wrote:
> > > Right, this is not correct. We need to iterate over the nodes in the
> pre-visit set and update //all// of them to generate `Dst`.
> > /me headdesks. Done. I'm still not sure I understand all the necessary
> pieces to generate new nodes (and have no idea how to write tests that
> trigger the corner cases), so I'd appreciate another careful look (or some
> test ideas).
> I'm not sure about the tests offhand, either—most likely it's currently
> untestable because we have no pre-statement checkers looking for
> CXXBindTemporaryExprs. The general principle, though, is that anything that
> has an ExplodedNodeSet out parameter has the potential to generate new
> nodes.
>
> ================
> Comment at: test/Analysis/temporaries.cpp:283-288
> @@ +282,8 @@
> +
> +  bool testRecursiveFrames(bool isInner) {
> +    if (isInner || check(NoReturnDtor()) || testRecursiveFrames(true)) {
> +      clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
> +    }
> +  }
> +  void testRecursiveFramesStart() { testRecursiveFrames(false); }
> +
> ----------------
> I know this was my example, but I'm concerned that testRecursiveFrames
> will be evaluated as top-level as well, in which case the inner case is
> trivially reached. Let's make sure it is trying the !isInner case around
> with a little hack:
>
>     if (isInner ||
>         (clang_analyzer_warnIfReached(), false) || //
> expected-warning{{REACHABLE}}
>         check(NoReturnDtor()) ||
>         testRecursiveFrames(true)) {
>

Done.


>
> http://reviews.llvm.org/D3627
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140612/8afb986d/attachment.html>