[PATCH] D15031: CFG: Add CFGElement for automatic variables that leave the scope

[Devin Coughlin via cfe-commits]

dcoughlin added a comment.

Thanks for patch! Some comments inline.

You don't have to do it in this patch, but I think it would be good to get this working with AddImplicitDtors. I think it would also be good to (eventually) add CFGElements marking when the storage duration for underlying storage ends. This would allow the static analyzer to warn when writing to storage that no longer exists, which is something I believe a.sidorin is working on in https://reviews.llvm.org/D19979.


================
Comment at: lib/Analysis/CFG.cpp:285
@@ +284,3 @@
+  const_iterator F = *this;
+  while(true) {
+    const_iterator LL = L;
----------------
I think you can make this linear time by walking up the spine of `*this` until you find either `L` or the root and caching the ancestors along the way. If you found `L`, you are done. If not, you can then walk up the spine from `L` until you find a scope in the cache.

While this approach is unlikely to be needed for human-written code, we do want to make sure to gracefully handle bizarre, deeply-nested scopes that have been programmatically generated.

================
Comment at: lib/Analysis/CFG.cpp:1091
@@ -1057,3 +1090,3 @@
   // encountered them.
   for (BackpatchBlocksTy::iterator I = BackpatchBlocks.begin(),
                                    E = BackpatchBlocks.end(); I != E; ++I ) {
----------------
I don't think you handle back-patched gotos yet. For example, consider:

```
void foo() {
  int i;
  label:
  B b;
  goto label;
  i++;
}
```
Here the lifetime of `b` should end immediately before the `goto` (right?) but your patch says it ends after `i++`.

================
Comment at: lib/Analysis/CFG.cpp:1260
@@ +1259,3 @@
+
+  // To from B to E, one first goes up the scopes from B to P
+  // then sideways in one scope from P to P' and then down
----------------
Nit: Missing word here. "To *go* from B to E ..."?


https://reviews.llvm.org/D15031