rsmith added a comment.
Please add a test that builds and dumps a CFG and checks it has the right
shape. It's generally not sufficient for the only tests for one LLVM project to
exist in a different LLVM project. See test/Analysis/cfg.cpp for an example of
how to do this.
================
Comment at: include/clang/Analysis/CFG.h:170
@@ -168,1 +169,3 @@
+class CFGAutomaticObjLeavesScope : public CFGElement {
+public:
----------------
What is this intended to model? There seem to be a few options here:
1) The point at which the destructor would run if the object had a non-trivial
destructor
2) The point at which the storage duration for the underlying storage ends
3) The point at which the lifetime of the object ends
Note that these are all different -- for an object with a non-trivial
destructor, (1) and (3) are the same and for any other object (2) and (3) are
the same; (2) occurs after all destructors for the scope have run.
This matters for cases like:
void f() {
struct A { int *p; ~A() { *p = 0; } } a;
int n;
a.p = &n;
}
Here, the lifetime of `n` would end before the lifetime of `a` if `n` had a
non-trivial destructor. If `n`s lifetime actually ended there, this code would
have undefined behavior. But because the destruction of `n` is trivial, the
lifetime of `n` instead ends when its storage duration ends, which is *after*
`A`'s destructor runs, so the code is valid.
Please add a comment to this class to describe what it means.
================
Comment at: include/clang/Analysis/CFG.h:728
@@ +727,3 @@
+
+ // Scope leaving must be performed in reversed order. So insertion is in two
+ // steps. First we prepare space for some number of elements, then we insert
----------------
Why must reverse order be used? It's not possible for the effects of these
operations to interact with each other, is it? At least according to the C++
standard, the "end of storage duration" effects all occur simultaneously.
================
Comment at: lib/Analysis/CFG.cpp:1233
@@ +1232,3 @@
+
+/// addAutomaticObjLeavesScope - Add to current block automatic objects thats
leave the scope.
+void CFGBuilder::addAutomaticObjLeavesScope(LocalScope::const_iterator B,
----------------
thats -> that
================
Comment at: lib/Analysis/CFG.cpp:1447
@@ -1389,1 +1446,3 @@
+ }
+ return Scope;
}
----------------
This will sometimes bail out without adding a scope; if both
`AddAutomaticObjLeavesScope` and `AddImplicitDtors` are set, this seems like
the wrong behavior. You should check for `AddAutomaticObjLeavesScope` first and
unconditionally ensure there is a scope for that case.
================
Comment at: lib/Analysis/CFG.cpp:1490-1491
@@ +1489,4 @@
+/// destructors call site.
+/// FIXME: This mechanism for adding automatic destructors doesn't handle
+/// no-return destructors properly.
+void CFGBuilder::prependAutomaticObjLeavesScopeWithTerminator(CFGBlock *Blk,
----------------
This is not a mechanism for adding automatic destructors. This FIXME seems
unnecessary.
================
Comment at: lib/Analysis/CFG.cpp:1501
@@ +1500,3 @@
+ InsertPos = Blk->insertAutomaticObjLeavesScope(InsertPos, *I,
+ Blk->getTerminator());
+}
----------------
Bad indentation
http://reviews.llvm.org/D15031
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
