Commit 60825c9b authored by Tudor Bosman's avatar Tudor Bosman Committed by Sara Golemon

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
parent bd0cc981
...@@ -336,6 +336,10 @@ IOBuf::IOBuf(IOBuf&& other) noexcept { ...@@ -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, IOBuf::IOBuf(InternalConstructor,
uintptr_t flagsAndSharedInfo, uintptr_t flagsAndSharedInfo,
uint8_t* buf, uint8_t* buf,
...@@ -413,6 +417,13 @@ IOBuf& IOBuf::operator=(IOBuf&& other) noexcept { ...@@ -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 { bool IOBuf::empty() const {
const IOBuf* current = this; const IOBuf* current = this;
do { do {
......
...@@ -189,13 +189,9 @@ namespace folly { ...@@ -189,13 +189,9 @@ namespace folly {
* an IOBuf chain must be heap allocated. (All functions to add nodes to a * 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.) * chain require a std::unique_ptr<IOBuf>, which enforces this requrement.)
* *
* Additionally, no copy-constructor or assignment operator currently exists, * Copying IOBufs is only meaningful for the head of a chain. The entire chain
* so stack-allocated IOBufs may only be moved, not copied. (Technically * is cloned; the IOBufs will become shared, and the old and new IOBufs will
* nothing is preventing us from adding a copy constructor and assignment * refer to the same underlying memory.
* 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.)
*
* *
* IOBuf Sharing * IOBuf Sharing
* ------------- * -------------
...@@ -232,7 +228,6 @@ class IOBuf { ...@@ -232,7 +228,6 @@ class IOBuf {
enum WrapBufferOp { WRAP_BUFFER }; enum WrapBufferOp { WRAP_BUFFER };
enum TakeOwnershipOp { TAKE_OWNERSHIP }; enum TakeOwnershipOp { TAKE_OWNERSHIP };
enum CopyBufferOp { COPY_BUFFER }; enum CopyBufferOp { COPY_BUFFER };
enum CloneOp { CLONE };
typedef ByteRange value_type; typedef ByteRange value_type;
typedef Iterator iterator; typedef Iterator iterator;
...@@ -398,13 +393,6 @@ class IOBuf { ...@@ -398,13 +393,6 @@ class IOBuf {
IOBuf(CopyBufferOp op, ByteRange br, IOBuf(CopyBufferOp op, ByteRange br,
uint64_t headroom=0, uint64_t minTailroom=0); 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 * Convenience function to create a new IOBuf object that copies data from a
* user-supplied string, optionally allocating a given amount of * user-supplied string, optionally allocating a given amount of
...@@ -1124,14 +1112,13 @@ class IOBuf { ...@@ -1124,14 +1112,13 @@ class IOBuf {
* the head of an IOBuf chain or a solitary IOBuf not part of a chain. If * 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 * the move destination is part of a chain, all other IOBufs in the chain
* will be deleted. * 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(IOBuf&& other) noexcept;
IOBuf& operator=(IOBuf&& other) noexcept; IOBuf& operator=(IOBuf&& other) noexcept;
IOBuf(const IOBuf& other);
IOBuf& operator=(const IOBuf& other);
private: private:
enum FlagsEnum : uintptr_t { enum FlagsEnum : uintptr_t {
// Adding any more flags would not work on 32-bit architectures, // Adding any more flags would not work on 32-bit architectures,
...@@ -1157,10 +1144,6 @@ class IOBuf { ...@@ -1157,10 +1144,6 @@ class IOBuf {
struct HeapStorage; struct HeapStorage;
struct HeapFullStorage; 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.
* *
......
...@@ -1108,6 +1108,50 @@ TEST(IOBuf, ReserveWithHeadroom) { ...@@ -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) { int main(int argc, char** argv) {
testing::InitGoogleTest(&argc, argv); testing::InitGoogleTest(&argc, argv);
gflags::ParseCommandLineFlags(&argc, &argv, true); gflags::ParseCommandLineFlags(&argc, &argv, true);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment