From 81f781c5d357abd262a61ac6fce1cb8f6962e8fe Mon Sep 17 00:00:00 2001 From: Adam Simpkins <simpkins@fb.com> Date: Thu, 2 Dec 2021 16:47:51 -0800 Subject: [PATCH] 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 --- folly/compression/test/CompressionTest.cpp | 4 +- folly/io/Cursor.h | 2 +- folly/io/IOBuf.cpp | 12 ++-- folly/io/IOBuf.h | 76 +++++++++++----------- folly/io/IOBufQueue.cpp | 2 +- folly/io/RecordIO.cpp | 2 +- folly/io/async/test/AsyncSocketTest2.cpp | 4 +- folly/io/test/IOBufCursorTest.cpp | 8 +-- folly/io/test/IOBufQueueTest.cpp | 2 +- folly/io/test/IOBufTest.cpp | 36 +++++----- folly/python/iobuf.pxd | 18 ++++- folly/python/test/iobuf_helper.pyx | 2 +- 12 files changed, 92 insertions(+), 76 deletions(-) diff --git a/folly/compression/test/CompressionTest.cpp b/folly/compression/test/CompressionTest.cpp index 5deb1b354..f9edea32c 100644 --- a/folly/compression/test/CompressionTest.cpp +++ b/folly/compression/test/CompressionTest.cpp @@ -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; } diff --git a/folly/io/Cursor.h b/folly/io/Cursor.h index b225757b6..7b024e80c 100644 --- a/folly/io/Cursor.h +++ b/folly/io/Cursor.h @@ -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. diff --git a/folly/io/IOBuf.cpp b/folly/io/IOBuf.cpp index 78fe03cba..ed263ce36 100644 --- a/folly/io/IOBuf.cpp +++ b/folly/io/IOBuf.cpp @@ -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)); } } } diff --git a/folly/io/IOBuf.h b/folly/io/IOBuf.h index ae09c565c..aad227a9f 100644 --- a/folly/io/IOBuf.h +++ b/folly/io/IOBuf.h @@ -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)); } /** diff --git a/folly/io/IOBufQueue.cpp b/folly/io/IOBufQueue.cpp index e5fbfd3f0..c9cf9ef17 100644 --- a/folly/io/IOBufQueue.cpp +++ b/folly/io/IOBufQueue.cpp @@ -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)); } } } diff --git a/folly/io/RecordIO.cpp b/folly/io/RecordIO.cpp index 653e0b60c..f90c9c439 100644 --- a/folly/io/RecordIO.cpp +++ b/folly/io/RecordIO.cpp @@ -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()); diff --git a/folly/io/async/test/AsyncSocketTest2.cpp b/folly/io/async/test/AsyncSocketTest2.cpp index 99902ae47..e9b504de1 100644 --- a/folly/io/async/test/AsyncSocketTest2.cpp +++ b/folly/io/async/test/AsyncSocketTest2.cpp @@ -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()); diff --git a/folly/io/test/IOBufCursorTest.cpp b/folly/io/test/IOBufCursorTest.cpp index 82a6b8d7e..9b9dad471 100644 --- a/folly/io/test/IOBufCursorTest.cpp +++ b/folly/io/test/IOBufCursorTest.cpp @@ -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()); diff --git a/folly/io/test/IOBufQueueTest.cpp b/folly/io/test/IOBufQueueTest.cpp index 6bf6511f2..26aacb4f1 100644 --- a/folly/io/test/IOBufQueueTest.cpp +++ b/folly/io/test/IOBufQueueTest.cpp @@ -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()); diff --git a/folly/io/test/IOBufTest.cpp b/folly/io/test/IOBufTest.cpp index 220ec1ad1..d9fa1e58d 100644 --- a/folly/io/test/IOBufTest.cpp +++ b/folly/io/test/IOBufTest.cpp @@ -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."; diff --git a/folly/python/iobuf.pxd b/folly/python/iobuf.pxd index ba029ba62..5b43c9dd7 100644 --- a/folly/python/iobuf.pxd +++ b/folly/python/iobuf.pxd @@ -1,3 +1,17 @@ +# 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": diff --git a/folly/python/test/iobuf_helper.pyx b/folly/python/test/iobuf_helper.pyx index 5e3744208..a992349bc 100644 --- a/folly/python/test/iobuf_helper.pyx +++ b/folly/python/test/iobuf_helper.pyx @@ -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 -- 2.26.2