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 7286f21df2bd98631017c819a0b9d41b371773b8..fb3c1b5c0d56689733632b503e2042f1d5c84ec1 100644 (file)
--- a/folly/io/IOBuf.cpp
+++ b/folly/io/IOBuf.cpp
@@ -336,6 +336,10 @@ IOBuf::IOBuf(IOBuf&& other) noexcept {
   *this = std::move(other);
 }
 
   *this = std::move(other);
 }
 

+IOBuf::IOBuf(const IOBuf& other) {
+  other.cloneInto(*this);
+}
+

 IOBuf::IOBuf(InternalConstructor,
              uintptr_t flagsAndSharedInfo,
              uint8_t* buf,
 IOBuf::IOBuf(InternalConstructor,
              uintptr_t flagsAndSharedInfo,
              uint8_t* buf,


@@ -413,6 +417,13 @@ IOBuf& IOBuf::operator=(IOBuf&& other) noexcept {
   return *this;
 }
 
   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 {
 bool IOBuf::empty() const {
   const IOBuf* current = this;
   do {

diff --git a/folly/io/IOBuf.h b/folly/io/IOBuf.h
index c75dbe1da07f7a89189f599ed84076b2a39203cc..a9c92c1e66238ee19b70cb70aa453585e2da8df6 100644 (file)
--- 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.)
  *
  * 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
  * -------------
  *
  * IOBuf Sharing
  * -------------


@@ -232,7 +228,6 @@ class IOBuf {
   enum WrapBufferOp { WRAP_BUFFER };
   enum TakeOwnershipOp { TAKE_OWNERSHIP };
   enum CopyBufferOp { COPY_BUFFER };
   enum WrapBufferOp { WRAP_BUFFER };
   enum TakeOwnershipOp { TAKE_OWNERSHIP };
   enum CopyBufferOp { COPY_BUFFER };

-  enum CloneOp { CLONE };

 
   typedef ByteRange value_type;
   typedef Iterator iterator;
 
   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);
 
   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
   /**
    * 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.
    * 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(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,
  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;
 
   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.
    *
   /**
    * Create a new IOBuf pointing to an external buffer.
    *

diff --git a/folly/io/test/IOBufTest.cpp b/folly/io/test/IOBufTest.cpp
index 0cc3ffd0e46bdd91b4961ca51784fdca9c32ef5d..4e4c1312825925f2f7676cf9b610a109c02bb085 100644 (file)
--- 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)));
 }
 
   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);
 int main(int argc, char** argv) {
   testing::InitGoogleTest(&argc, argv);
   gflags::ParseCommandLineFlags(&argc, &argv, true);