From: Tudor Bosman <tudorb@fb.com>
Date: Thu, 11 Jun 2015 18:18:41 +0000 (-0700)
Subject: Make IOBuf copyable
X-Git-Tag: v0.47.0~40
X-Git-Url: http://plrg.eecs.uci.edu/git/?p=folly.git;a=commitdiff_plain;h=60825c9bca47c6919ad192527dacd052b02e13b9

Make IOBuf copyable

Summary: Give IOBuf a copy constructor, which clones the whole chain. (IOBufs have
shared pointer semantics). You could argue for cloning one single IOBuf,
but cloning the whole chain has the nice side effect of making Thrift
structures with IOBufs in them copyable, so there's that.

Reviewed By: @djwatson

Differential Revision: D2127209
---

diff --git a/folly/io/IOBuf.cpp b/folly/io/IOBuf.cpp
index 7286f21d..fb3c1b5c 100644
--- a/folly/io/IOBuf.cpp
+++ b/folly/io/IOBuf.cpp
@@ -336,6 +336,10 @@ IOBuf::IOBuf(IOBuf&& other) noexcept {
   *this = std::move(other);
 }
 
+IOBuf::IOBuf(const IOBuf& other) {
+  other.cloneInto(*this);
+}
+
 IOBuf::IOBuf(InternalConstructor,
              uintptr_t flagsAndSharedInfo,
              uint8_t* buf,
@@ -413,6 +417,13 @@ IOBuf& IOBuf::operator=(IOBuf&& other) noexcept {
   return *this;
 }
 
+IOBuf& IOBuf::operator=(const IOBuf& other) {
+  if (this != &other) {
+    *this = IOBuf(other);
+  }
+  return *this;
+}
+
 bool IOBuf::empty() const {
   const IOBuf* current = this;
   do {
diff --git a/folly/io/IOBuf.h b/folly/io/IOBuf.h
index c75dbe1d..a9c92c1e 100644
--- a/folly/io/IOBuf.h
+++ b/folly/io/IOBuf.h
@@ -189,13 +189,9 @@ namespace folly {
  * an IOBuf chain must be heap allocated.  (All functions to add nodes to a
  * chain require a std::unique_ptr<IOBuf>, which enforces this requrement.)
  *
- * Additionally, no copy-constructor or assignment operator currently exists,
- * so stack-allocated IOBufs may only be moved, not copied.  (Technically
- * nothing is preventing us from adding a copy constructor and assignment
- * operator.  However, it seems like this would add the possibility for some
- * confusion.  We would need to determine if these functions would copy just a
- * single buffer, or the entire chain.)
- *
+ * Copying IOBufs is only meaningful for the head of a chain. The entire chain
+ * is cloned; the IOBufs will become shared, and the old and new IOBufs will
+ * refer to the same underlying memory.
  *
  * IOBuf Sharing
  * -------------
@@ -232,7 +228,6 @@ class IOBuf {
   enum WrapBufferOp { WRAP_BUFFER };
   enum TakeOwnershipOp { TAKE_OWNERSHIP };
   enum CopyBufferOp { COPY_BUFFER };
-  enum CloneOp { CLONE };
 
   typedef ByteRange value_type;
   typedef Iterator iterator;
@@ -398,13 +393,6 @@ class IOBuf {
   IOBuf(CopyBufferOp op, ByteRange br,
         uint64_t headroom=0, uint64_t minTailroom=0);
 
-  /**
-   * Clone an IOBuf. See the notes for cloneInto().
-   */
-  IOBuf(CloneOp, const IOBuf& src) : IOBuf() {
-    src.cloneInto(*this);
-  }
-
   /**
    * Convenience function to create a new IOBuf object that copies data from a
    * user-supplied string, optionally allocating a given amount of
@@ -1124,14 +1112,13 @@ class IOBuf {
    * the head of an IOBuf chain or a solitary IOBuf not part of a chain.  If
    * the move destination is part of a chain, all other IOBufs in the chain
    * will be deleted.
-   *
-   * (We currently don't provide a copy constructor or assignment operator.
-   * The main reason is because it is not clear these operations should copy
-   * the entire chain or just the single IOBuf.)
    */
   IOBuf(IOBuf&& other) noexcept;
   IOBuf& operator=(IOBuf&& other) noexcept;
 
+  IOBuf(const IOBuf& other);
+  IOBuf& operator=(const IOBuf& other);
+
  private:
   enum FlagsEnum : uintptr_t {
     // Adding any more flags would not work on 32-bit architectures,
@@ -1157,10 +1144,6 @@ class IOBuf {
   struct HeapStorage;
   struct HeapFullStorage;
 
-  // Forbidden copy constructor and assignment opererator
-  IOBuf(IOBuf const &);
-  IOBuf& operator=(IOBuf const &);
-
   /**
    * Create a new IOBuf pointing to an external buffer.
    *
diff --git a/folly/io/test/IOBufTest.cpp b/folly/io/test/IOBufTest.cpp
index 0cc3ffd0..4e4c1312 100644
--- a/folly/io/test/IOBufTest.cpp
+++ b/folly/io/test/IOBufTest.cpp
@@ -1108,6 +1108,50 @@ TEST(IOBuf, ReserveWithHeadroom) {
   EXPECT_EQ(0, memcmp(data, buf.data(), sizeof(data)));
 }
 
+TEST(IOBuf, CopyConstructorAndAssignmentOperator) {
+  auto buf = IOBuf::create(4096);
+  append(buf, "hello world");
+  auto buf2 = IOBuf::create(4096);
+  append(buf2, " goodbye");
+  buf->prependChain(std::move(buf2));
+  EXPECT_FALSE(buf->isShared());
+
+  {
+    auto copy = *buf;
+    EXPECT_TRUE(buf->isShared());
+    EXPECT_TRUE(copy.isShared());
+    EXPECT_EQ((void*)buf->data(), (void*)copy.data());
+    EXPECT_NE(buf->next(), copy.next());  // actually different buffers
+
+    auto copy2 = *buf;
+    copy2.coalesce();
+    EXPECT_TRUE(buf->isShared());
+    EXPECT_TRUE(copy.isShared());
+    EXPECT_FALSE(copy2.isShared());
+
+    auto p = reinterpret_cast<const char*>(copy2.data());
+    EXPECT_EQ("hello world goodbye", std::string(p, copy2.length()));
+  }
+
+  EXPECT_FALSE(buf->isShared());
+
+  {
+    folly::IOBuf newBuf(folly::IOBuf::CREATE, 4096);
+    EXPECT_FALSE(newBuf.isShared());
+
+    auto newBufCopy = newBuf;
+    EXPECT_TRUE(newBuf.isShared());
+    EXPECT_TRUE(newBufCopy.isShared());
+
+    newBufCopy = *buf;
+    EXPECT_TRUE(buf->isShared());
+    EXPECT_FALSE(newBuf.isShared());
+    EXPECT_TRUE(newBufCopy.isShared());
+  }
+
+  EXPECT_FALSE(buf->isShared());
+}
+
 int main(int argc, char** argv) {
   testing::InitGoogleTest(&argc, argv);
   gflags::ParseCommandLineFlags(&argc, &argv, true);