Commit 81f781c5 authored by Adam Simpkins's avatar Adam Simpkins Committed by Facebook GitHub Bot

start renaming IOBuf::prependChain() to IOBuf::appendToChain()

Summary:
The `prependChain()` and `appendChain()` method names have always been
confusing, ever since they were first added in D271954.

These methods prepend and append immediately before or after the current
element.  However, because IOBuf chains are circular, this effectively means
that `prependChain()` appends to the very end of the chain when called on the
head of an IOBuf chain.  This is almost always how it is used in practice, so
having it be named `prependChain()` is quite confusing.  (Note that in the
initial version of D271954 these methods were originally named `appendChain()`
and `appendChainHere()`, but were unfortunately renamed during review.)

This diff adds alternate names for these methods, so we can migrate call sites
to use the new names, and then deprecate these existing names.

`appendToChain()` is the new recommended name for `prependChain()`, and
`insertAfterThisOne()` is the new recommended name for `appendChain()`

Reviewed By: voznesenskym

Differential Revision: D32524091

fbshipit-source-id: faf74f8803947c6452a10b58ced0c38efbd320fd
parent c424777a
......@@ -1060,7 +1060,7 @@ void AutomaticCodecTest::runSimpleTest(const DataHolder& dh) {
auto rest = compressed->clone();
split->trimEnd(split->length() - i);
rest->trimStart(i);
split->appendChain(std::move(rest));
split->insertAfterThisOne(std::move(rest));
auto uncompressed = auto_->uncompress(split.get(), uncompressedLength);
EXPECT_EQ(uncompressedLength, uncompressed->computeChainDataLength());
EXPECT_EQ(dh.hash(uncompressedLength), hashIOBuf(uncompressed.get()));
......@@ -1145,7 +1145,7 @@ class CustomCodec : public Codec {
std::unique_ptr<IOBuf> doCompress(const IOBuf* data) override {
auto result = IOBuf::copyBuffer(prefix_);
result->appendChain(codec_->compress(data));
result->appendToChain(codec_->compress(data));
EXPECT_TRUE(canUncompress(result.get(), data->computeChainDataLength()));
return result;
}
......
......@@ -998,7 +998,7 @@ class RWCursor : public detail::CursorBase<RWCursor<access>, IOBuf>,
}
this->crtBuf_->trimEnd(this->length());
this->absolutePos_ += this->crtPos_ - this->crtBegin_;
this->crtBuf_->appendChain(std::move(buf));
this->crtBuf_->insertAfterThisOne(std::move(buf));
if (nextBuf == this->buffer_) {
// We've just appended to the end of the buffer, so advance to the end.
......
......@@ -383,7 +383,7 @@ unique_ptr<IOBuf> IOBuf::createChain(
unique_ptr<IOBuf> newBuf = create(
std::min(totalCapacity - allocatedCapacity, size_t(maxBufCapacity)));
allocatedCapacity += newBuf->capacity();
out->prependChain(std::move(newBuf));
out->appendToChain(std::move(newBuf));
}
return out;
......@@ -702,7 +702,7 @@ std::size_t IOBuf::computeChainCapacity() const {
return fullCapacity;
}
void IOBuf::prependChain(unique_ptr<IOBuf>&& iobuf) {
void IOBuf::appendToChain(unique_ptr<IOBuf>&& iobuf) {
// Take ownership of the specified IOBuf
IOBuf* other = iobuf.release();
......@@ -724,7 +724,7 @@ unique_ptr<IOBuf> IOBuf::clone() const {
auto tmp = cloneOne();
for (IOBuf* current = next_; current != this; current = current->next_) {
tmp->prependChain(current->cloneOne());
tmp->appendToChain(current->cloneOne());
}
return tmp;
......@@ -757,7 +757,7 @@ IOBuf IOBuf::cloneAsValue() const {
auto tmp = cloneOneAsValue();
for (IOBuf* current = next_; current != this; current = current->next_) {
tmp.prependChain(current->cloneOne());
tmp.appendToChain(current->cloneOne());
}
return tmp;
......@@ -1333,7 +1333,7 @@ unique_ptr<IOBuf> IOBuf::wrapIov(const iovec* vec, size_t count) {
if (!result) {
result = std::move(buf);
} else {
result->prependChain(std::move(buf));
result->appendToChain(std::move(buf));
}
}
}
......@@ -1358,7 +1358,7 @@ std::unique_ptr<IOBuf> IOBuf::takeOwnershipIov(
if (!result) {
result = std::move(buf);
} else {
result->prependChain(std::move(buf));
result->appendToChain(std::move(buf));
}
}
}
......
......@@ -133,8 +133,9 @@ namespace folly {
* chain is deleted, all other chained elements are also deleted. Conceptually
* it is simplest to treat this as if the head of the chain owns all other
* IOBufs in the chain. When you delete the head of the chain, it will delete
* the other elements as well. For this reason, prependChain() and
* appendChain() take ownership of the new elements being added to this chain.
* the other elements as well. For this reason, appendToChain() and
* insertAfterThisOne() take ownership of the new elements being added to this
* chain.
*
* When the coalesce() method is used to coalesce an entire IOBuf chain into a
* single IOBuf, all other IOBufs in the chain are eliminated and automatically
......@@ -860,54 +861,53 @@ class IOBuf {
std::size_t computeChainCapacity() const;
/**
* Insert another IOBuf chain immediately before this IOBuf.
* Append another IOBuf chain to the end of this chain.
*
* For example, if there are two IOBuf chains (A, B, C) and (D, E, F),
* and B->prependChain(D) is called, the (D, E, F) chain will be subsumed
* and A->appendToChain(D) is called, the (D, E, F) chain will be subsumed
* and become part of the chain starting at A, which will now look like
* (A, D, E, F, B, C)
*
* Note that since IOBuf chains are circular, head->prependChain(other) can
* be used to append the other chain at the very end of the chain pointed to
* by head. For example, if there are two IOBuf chains (A, B, C) and
* (D, E, F), and A->prependChain(D) is called, the chain starting at A will
* now consist of (A, B, C, D, E, F)
*
* The elements in the specified IOBuf chain will become part of this chain,
* and will be owned by the head of this chain. When this chain is
* destroyed, all elements in the supplied chain will also be destroyed.
*
* For this reason, appendChain() only accepts an rvalue-reference to a
* unique_ptr(), to make it clear that it is taking ownership of the supplied
* chain. If you have a raw pointer, you can pass in a new temporary
* unique_ptr around the raw pointer. If you have an existing,
* non-temporary unique_ptr, you must call std::move(ptr) to make it clear
* that you are destroying the original pointer.
* (A, B, C, D, E, F)
*/
void prependChain(std::unique_ptr<IOBuf>&& iobuf);
void appendToChain(std::unique_ptr<IOBuf>&& iobuf);
/**
* Append another IOBuf chain immediately after this IOBuf.
* Insert an IOBuf chain immediately after this chain element.
*
* For example, if there are two IOBuf chains (A, B, C) and (D, E, F),
* and B->appendChain(D) is called, the (D, E, F) chain will be subsumed
* and become part of the chain starting at A, which will now look like
* (A, B, D, E, F, C)
* and B->insertAfterThisOne(D) is called, the (D, E, F) chain will be
* subsumed and become part of the chain starting at A, which will now look
* like (A, B, D, E, F, C)
*
* The elements in the specified IOBuf chain will become part of this chain,
* and will be owned by the head of this chain. When this chain is
* destroyed, all elements in the supplied chain will also be destroyed.
* Note if X is an IOBuf chain with just a single element, X->appendToChain()
* and X->insertAfterThisOne() behave identically.
*/
void insertAfterThisOne(std::unique_ptr<IOBuf>&& iobuf) {
// Just use appendToChain() on the next element in our chain
next_->appendToChain(std::move(iobuf));
}
/**
* Deprecated name for appendToChain()
*
* IOBuf chains are circular, so appending to the end of the chain is
* logically equivalent to prepending to the current head (but keeping the
* chain head pointing to the same element). That was the reason this method
* was originally called prependChain(). However, almost every time this
* method is called the intent is to append to the end of a chain, so the
* `prependChain()` name is very confusing to most callers.
*/
void prependChain(std::unique_ptr<IOBuf>&& iobuf) {
appendToChain(std::move(iobuf));
}
/**
* Deprecated name for insertAfterThisOne()
*
* For this reason, appendChain() only accepts an rvalue-reference to a
* unique_ptr(), to make it clear that it is taking ownership of the supplied
* chain. If you have a raw pointer, you can pass in a new temporary
* unique_ptr around the raw pointer. If you have an existing,
* non-temporary unique_ptr, you must call std::move(ptr) to make it clear
* that you are destroying the original pointer.
* Beware: appendToChain() and appendChain() are two different methods,
* and you probably want appendToChain() instead of this one.
*/
void appendChain(std::unique_ptr<IOBuf>&& iobuf) {
// Just use prependChain() on the next element in our chain
next_->prependChain(std::move(iobuf));
insertAfterThisOne(std::move(iobuf));
}
/**
......
......@@ -63,7 +63,7 @@ void appendToChain(unique_ptr<IOBuf>& dst, unique_ptr<IOBuf>&& src, bool pack) {
packInto(tail, src, [](auto&& cur) { return cur->pop(); });
}
if (src) {
tail->appendChain(std::move(src));
tail->insertAfterThisOne(std::move(src));
}
}
}
......
......@@ -153,7 +153,7 @@ size_t prependHeader(std::unique_ptr<IOBuf>& buf, uint32_t fileId) {
} else {
auto b = IOBuf::create(headerSize());
b->append(headerSize());
b->appendChain(std::move(buf));
b->insertAfterThisOne(std::move(buf));
buf = std::move(b);
}
auto header = reinterpret_cast<Header*>(buf->writableData());
......
......@@ -1437,7 +1437,7 @@ TEST(AsyncSocketTest, WriteIOBuf) {
unique_ptr<IOBuf> buf3(IOBuf::create(buf3Length));
memset(buf3->writableData(), 'd', buf3Length);
buf3->append(buf3Length);
buf2->appendChain(std::move(buf3));
buf2->appendToChain(std::move(buf3));
unique_ptr<IOBuf> buf2Copy(buf2->clone());
buf2Copy->coalesce();
WriteCallback wcb3;
......@@ -2542,7 +2542,7 @@ TEST(AsyncSocketTest, BufferTestChain) {
memset(buf2, 'f', sizeof(buf2));
auto buf = folly::IOBuf::copyBuffer(buf1, sizeof(buf1));
buf->appendChain(folly::IOBuf::copyBuffer(buf2, sizeof(buf2)));
buf->appendToChain(folly::IOBuf::copyBuffer(buf2, sizeof(buf2)));
ASSERT_EQ(sizeof(buf1) + sizeof(buf2), buf->computeChainDataLength());
BufferCallback bcb(socket.get(), buf->computeChainDataLength());
......
......@@ -699,7 +699,7 @@ TEST(IOBuf, CursorOperators) {
{
std::unique_ptr<IOBuf> chain(IOBuf::create(20));
chain->append(10);
chain->appendChain(chain->clone());
chain->appendToChain(chain->clone());
EXPECT_EQ(20, chain->computeChainDataLength());
Cursor curs1(chain.get());
......@@ -744,7 +744,7 @@ TEST(IOBuf, CursorOperators) {
{
auto chain = IOBuf::create(10);
chain->append(10);
chain->appendChain(chain->clone());
chain->appendToChain(chain->clone());
EXPECT_EQ(2, chain->countChainElements());
EXPECT_EQ(20, chain->computeChainDataLength());
......@@ -1258,7 +1258,7 @@ TEST(IOBuf, BoundedCursorSanity) {
EXPECT_THROW(subC.skip(1), std::out_of_range);
// multi-item chain
chain1->appendChain(chain1->clone());
chain1->appendToChain(chain1->clone());
EXPECT_EQ(2, chain1->countChainElements());
EXPECT_EQ(20, chain1->computeChainDataLength());
......@@ -1458,7 +1458,7 @@ TEST(IOBuf, BoundedCursorOperators) {
{
auto chain = IOBuf::create(10);
chain->append(10);
chain->appendChain(chain->clone());
chain->appendToChain(chain->clone());
EXPECT_EQ(2, chain->countChainElements());
EXPECT_EQ(20, chain->computeChainDataLength());
......
......@@ -561,7 +561,7 @@ TEST(IOBufQueue, ReuseTail) {
queue.append("hello ");
buf = stringToIOBuf(SCL("world"));
packableLength = buf->length();
buf->appendChain(std::move(unpackable));
buf->insertAfterThisOne(std::move(unpackable));
}
const auto oldTail = reinterpret_cast<uint8_t*>(queue.writableTail());
......
......@@ -443,7 +443,7 @@ TEST(IOBuf, Chaining) {
iob1->prependChain(std::move(iob2));
iob1->prependChain(std::move(iob4));
iob2ptr->appendChain(std::move(iob3));
iob2ptr->insertAfterThisOne(std::move(iob3));
iob1->prependChain(std::move(iob5));
EXPECT_EQ(iob2ptr, iob1->next());
......@@ -640,9 +640,9 @@ TEST(IOBuf, Chaining) {
iob4 = IOBuf::create(7);
iob4->append(7);
iob4ptr = iob4.get();
iob1->appendChain(std::move(iob2));
iob1->prev()->appendChain(std::move(iob3));
iob1->prev()->appendChain(std::move(iob4));
iob1->insertAfterThisOne(std::move(iob2));
iob1->prev()->insertAfterThisOne(std::move(iob3));
iob1->prev()->insertAfterThisOne(std::move(iob4));
EXPECT_EQ(4, iob1->countChainElements());
EXPECT_EQ(16, iob1->computeChainDataLength());
......@@ -1523,8 +1523,8 @@ TEST(IOBuf, CoalesceEmptyBuffers) {
auto b2 = fromStr("hello");
auto b3 = IOBuf::takeOwnership(nullptr, 0);
b2->appendChain(std::move(b3));
b1->appendChain(std::move(b2));
b2->insertAfterThisOne(std::move(b3));
b1->insertAfterThisOne(std::move(b2));
auto br = b1->coalesce();
......@@ -1617,8 +1617,8 @@ TEST(IOBuf, fillIov) {
auto buf3 = IOBuf::create(4096);
append(buf3, "hello again");
buf2->appendChain(std::move(buf3));
buf->appendChain(std::move(buf2));
buf2->insertAfterThisOne(std::move(buf3));
buf->insertAfterThisOne(std::move(buf2));
constexpr size_t iovCount = 3;
struct iovec vec[iovCount];
......@@ -1648,8 +1648,8 @@ TEST(IOBuf, fillIov2) {
auto buf3 = IOBuf::create(4096);
append(buf2, "hello again");
buf2->appendChain(std::move(buf3));
buf->appendChain(std::move(buf2));
buf2->insertAfterThisOne(std::move(buf3));
buf->insertAfterThisOne(std::move(buf2));
constexpr size_t iovCount = 2;
struct iovec vec[iovCount];
......@@ -1767,15 +1767,15 @@ TEST(IOBuf, computeChainCapacityOfMixedCapacityChainedIOBuf) {
// Create IOBuf chains
auto temp = buf.get();
temp->appendChain(IOBuf::wrapBuffer(data2, sizeof(data2)));
temp->insertAfterThisOne(IOBuf::wrapBuffer(data2, sizeof(data2)));
temp = temp->next();
temp->appendChain(IOBuf::wrapBuffer(data3, sizeof(data3)));
temp->insertAfterThisOne(IOBuf::wrapBuffer(data3, sizeof(data3)));
temp = temp->next();
temp->appendChain(IOBuf::wrapBuffer(data4, sizeof(data4)));
temp->insertAfterThisOne(IOBuf::wrapBuffer(data4, sizeof(data4)));
temp = temp->next();
temp->appendChain(IOBuf::wrapBuffer(data5, sizeof(data5)));
temp->insertAfterThisOne(IOBuf::wrapBuffer(data5, sizeof(data5)));
temp = temp->next();
temp->appendChain(IOBuf::wrapBuffer(data6, sizeof(data6)));
temp->insertAfterThisOne(IOBuf::wrapBuffer(data6, sizeof(data6)));
EXPECT_EQ(buf->computeChainCapacity(), 100);
}
......@@ -1787,11 +1787,11 @@ TEST(IOBuf, AppendTo) {
EXPECT_EQ(buf.to<std::string>(), "");
auto temp = &buf;
temp->appendChain(IOBuf::copyBuffer("Hello"));
temp->insertAfterThisOne(IOBuf::copyBuffer("Hello"));
temp = temp->next();
temp->appendChain(IOBuf::copyBuffer(" and"));
temp->insertAfterThisOne(IOBuf::copyBuffer(" and"));
temp = temp->next();
temp->appendChain(IOBuf::copyBuffer(" goodbye."));
temp->insertAfterThisOne(IOBuf::copyBuffer(" goodbye."));
auto testAppendTo = [&](auto c) {
const StringPiece kExpected = "Hello and goodbye.";
......
# Copyright (c) Facebook, Inc. and its affiliates.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
# distutils: language = c++
from libcpp.string cimport string
......@@ -19,7 +33,9 @@ cdef extern from "folly/io/IOBuf.h" namespace "folly":
unique_ptr[cIOBuf] clone()
cIOBuf* prev()
cIOBuf* next()
void appendChain(unique_ptr[cIOBuf]&& ciobuf)
void insertAfterThisOne(unique_ptr[cIOBuf]&& ciobuf)
void appendToChain(unique_ptr[cIOBuf]&& ciobuf)
void appendChain(unique_ptr[cIOBuf]&& ciobuf) # deprecated
cdef extern from "folly/io/IOBuf.h" namespace "folly::IOBuf":
......
......@@ -26,6 +26,6 @@ def make_chain(data):
cdef IOBuf tbuf
cdef IOBuf last = head
for tbuf in data:
last._this.appendChain(tbuf.c_clone())
last._this.insertAfterThisOne(tbuf.c_clone())
last = last.next
return head
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