From 8fbe2f85fa1f5ea0d20a8561d9def2ad72edb618 Mon Sep 17 00:00:00 2001
From: Chad Austin <chadaustin@fb.com>
Date: Fri, 9 Mar 2018 13:32:42 -0800
Subject: [PATCH] add explicit conversion from std::unique_ptr to InodePtr

Summary:
To make it clearer why newPtrLocked is safe during the Inode class
construction process, add a takeOwnership function (better name
suggestions welcome) that converts from the newly-instantiated
unique_ptr to a managed InodePtr.

Reviewed By: simpkins

Differential Revision: D7204818

fbshipit-source-id: 8a40189588442490120623da86195c6fc99c9c51
---
 eden/fs/inodes/InodePtr.h    | 15 +++++++++++++--
 eden/fs/inodes/TreeInode.cpp | 12 ++++++------
 2 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/eden/fs/inodes/InodePtr.h b/eden/fs/inodes/InodePtr.h
index 17154cc5bf..8a23ad94e0 100644
--- a/eden/fs/inodes/InodePtr.h
+++ b/eden/fs/inodes/InodePtr.h
@@ -13,6 +13,7 @@
 
 #include <glog/logging.h>
 #include <cstddef>
+#include <memory>
 #include <utility>
 
 namespace facebook {
@@ -126,6 +127,13 @@ class InodePtrImpl {
     return InodePtrImpl{value, LOCKED_INCREMENT};
   }
 
+  /**
+   * Like newPtrLocked() but consumes the given unique_ptr.
+   */
+  static InodePtrImpl takeOwnership(std::unique_ptr<InodeType> value) noexcept {
+    return InodePtrImpl{value.release(), LOCKED_INCREMENT};
+  }
+
   /**
    * An API for TreeInode to use to construct an InodePtr from itself in order
    * to give to new children inodes that it creates.
@@ -234,12 +242,15 @@ class InodePtr : public InodePtrImpl<InodeBase> {
   }
 
   /*
-   * Override newPtrLocked() and newPtrFromExisting() to return an
-   * InodePtr instead of the InodePtrImpl parent class.
+   * Override newPtrLocked(), takeOwnership(), and newPtrFromExisting() to
+   * return an InodePtr instead of the InodePtrImpl parent class.
    */
   static InodePtr newPtrLocked(InodeBase* value) noexcept {
     return InodePtr{value, InodePtrImpl<InodeBase>::LOCKED_INCREMENT};
   }
+  static InodePtr takeOwnership(std::unique_ptr<InodeBase> value) noexcept {
+    return InodePtr::newPtrLocked(value.release());
+  }
   static InodePtr newPtrFromExisting(InodeBase* value) noexcept {
     return InodePtr{value, InodePtrImpl<InodeBase>::NORMAL_INCREMENT};
   }
diff --git a/eden/fs/inodes/TreeInode.cpp b/eden/fs/inodes/TreeInode.cpp
index 76a95b724d..ca631bfbd5 100644
--- a/eden/fs/inodes/TreeInode.cpp
+++ b/eden/fs/inodes/TreeInode.cpp
@@ -243,7 +243,7 @@ Future<InodePtr> TreeInode::getOrLoadChild(PathComponentPiece name) {
         auto childInode = loadFuture.get();
         entryPtr.setInode(childInode.get());
         promises = getInodeMap()->inodeLoadComplete(childInode.get());
-        childInodePtr = InodePtr::newPtrLocked(childInode.release());
+        childInodePtr = InodePtr::takeOwnership(std::move(childInode));
       } else {
         inodeLoadFuture = std::move(loadFuture);
       }
@@ -310,11 +310,11 @@ class LookupProcessor {
 Future<InodePtr> TreeInode::getChildRecursive(RelativePathPiece path) {
   auto pathStr = path.stringPiece();
   if (pathStr.empty()) {
-    return makeFuture<InodePtr>(InodePtr::newPtrFromExisting(this));
+    return makeFuture<InodePtr>(inodePtrFromThis());
   }
 
   auto processor = std::make_unique<LookupProcessor>(path);
-  auto future = processor->next(TreeInodePtr::newPtrFromExisting(this));
+  auto future = processor->next(inodePtrFromThis());
   // This ensure() callback serves to hold onto the unique_ptr,
   // and makes sure it only gets destroyed when the future is finally resolved.
   return future.ensure([p = std::move(processor)]() mutable { p.reset(); });
@@ -354,7 +354,7 @@ void TreeInode::loadUnlinkedChildInode(
       auto file = std::make_unique<FileInode>(
           number, inodePtrFromThis(), name, mode, hash);
       promises = getInodeMap()->inodeLoadComplete(file.get());
-      inodePtr = InodePtr::newPtrLocked(file.release());
+      inodePtr = InodePtr::takeOwnership(std::move(file));
     } else {
       Dir dir;
 
@@ -380,7 +380,7 @@ void TreeInode::loadUnlinkedChildInode(
       auto tree = std::make_unique<TreeInode>(
           number, inodePtrFromThis(), name, std::move(dir));
       promises = getInodeMap()->inodeLoadComplete(tree.get());
-      inodePtr = InodePtr::newPtrLocked(tree.release());
+      inodePtr = InodePtr::takeOwnership(std::move(tree));
     }
 
     inodePtr->markUnlinkedAfterLoad();
@@ -489,7 +489,7 @@ void TreeInode::inodeLoadComplete(
   }
 
   // Fulfill all of the pending promises after releasing our lock
-  auto inodePtr = InodePtr::newPtrLocked(childInode.release());
+  auto inodePtr = InodePtr::takeOwnership(std::move(childInode));
   for (auto& promise : promises) {
     promise.setValue(inodePtr);
   }