[PATCH] Implement inlining of temporary destructors.

[Manuel Klimek]

On Sat, Aug 9, 2014 at 3:16 AM, Jordan Rose <jordan_rose at apple.com> wrote:

> Do we really need to put the "binds parameter" information in the CFG? I
> was hoping we could look in the parent map to see how the temporary is used
> (like for 'new' right now).
>

Ah, I didn't know there was a parent map already. I'll look into it.


>
> ================
> Comment at: include/clang/Analysis/CFG.h:285
> @@ -285,1 +284,3 @@
> +  CFGTemporaryDtor(CXXBindTemporaryExpr *expr, bool BindsParameter)
> +      : CFGImplicitDtor(TemporaryDtor, expr, BindsParameter ? T :
> nullptr) {}
>
> ----------------
> Why not just use "self"? Or &T? Why "T"? Why does T require a dynamic
> allocation?
>

Because I hoped you would say something like the above :) and I didn't find
a good way to add a bool (the PointerIntPair Data2 already uses the "int"
part for the kind, and it doesn't work with pointers to static constants
(throws a runtime error in PointerInPair for incorrectly aligned pointer)).


>
> ================
> Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:688-693
> @@ +687,8 @@
> +  const MemRegion *Region = nullptr;
> +  // If the class does not have any members, there will not be a region
> +  // for it bound in the environment.
> +  if (Optional<nonloc::LazyCompoundVal> LCV =
> +          Val.getAs<nonloc::LazyCompoundVal>()) {
> +    Region = LCV->getRegion();
> +  }
> +
> ----------------
> This is not how LCVs work. An LCV is a snapshot of a region at a
> particular "time" (state of the Store); the region may not exist in the
> current store. The "best" answer in the current scheme is to use
> createTemporaryRegionIfNeeded, but the right answer may end up being to
> disregard the rvalue-ness of temporaries (as we talked about some on
> cfe-dev).
>

Can you give me test cases that fail so I can work against them?


> ================
> Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:508-509
> @@ -507,13 +507,4 @@
>
>  bool ScanReachableSymbols::scan(nonloc::LazyCompoundVal val) {
> -  bool wasVisited = !visited.insert(val.getCVData()).second;
> -  if (wasVisited)
> -    return true;
> -
> -  StoreManager &StoreMgr = state->getStateManager().getStoreManager();
> -  // FIXME: We don't really want to use getBaseRegion() here because
> pointer
> -  // arithmetic doesn't apply, but scanReachableSymbols only accepts base
> -  // regions right now.
> -  const MemRegion *R = val.getRegion()->getBaseRegion();
> -  return StoreMgr.scanReachableSymbols(val.getStore(), R, *this);
> +  return scan(val.getRegion());
>  }
> ----------------
> This is not correct; see above.


Can you explain that in more detail - especially for the scan part? What
does this break?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140809/c8e2170c/attachment.html>