[PATCH] D15911: Move ownership of Action objects into Compilation.

[Justin Lebar via cfe-commits]

jlebar added inline comments.

================
Comment at: include/clang/Driver/Action.h:36
@@ -35,1 +35,3 @@
+///
+/// Actions are usually owned by a Compilation.
 class Action {
----------------
tra wrote:
> There's no API to pass ownership to Compilation explicitly, so the only way for an Action to be owned by Compilation is to create it with MakeAction. 
> 
> Perhaps "Actions created with MakeAction<>() are owned by Compilation"
> 
> BTW, should we (can we?) make MakeAction<>() the only way to create actions?
Updated the comment.

================
Comment at: include/clang/Driver/Compilation.h:118
@@ +117,3 @@
+  template <typename T, typename... Args> T *MakeAction(Args &&... Arg) {
+    static_assert(std::is_convertible<T *, Action *>::value,
+                  "T must be derived from Action.");
----------------
dblaikie wrote:
> Did you find this static_assert to be particularly helpful? Similar errors (though somewhat more verbose) would be produced during the push_back call a few lines down anyway.
Fair enough.  I got rid of it but changed my unique_ptr into a unique_ptr<Action> so that we'll get an error somewhere other than inside the bowels of SmallVector.

================
Comment at: include/clang/Driver/Compilation.h:120
@@ +119,3 @@
+                  "T must be derived from Action.");
+    std::unique_ptr<T> A = llvm::make_unique<T>(std::forward<Args>(Arg)...);
+    T* RawPtr = A.get();
----------------
dblaikie wrote:
> Up to you, but we usually just drop the "std::unique_ptr<T>" in favor of "auto" when the RHS has "make_unique".
Done, but then I realized I can just use operator new and skirt this entirely.


http://reviews.llvm.org/D15911