tape: buider rework (#2133)

Summary:
Pull Request resolved: #2133

There was some feedback about current design:
* it would be great for a commit to be explicit.
* conditionally extending or starting a new record sometimes useful.

## Alternative

* Explicit autocommit on destructor

We can continue to do this. The problem is: empty range, at the moment empty range is the same as any other. If you have a commit function then

```
builder =  ...;
builder.push_back('a');
builder.commit();
```
should only add "a".

I don't think it's a good idea basically.

* Move semantics to builder.

Like unique_ptr - builder. When goes out of scope - commit happens.
Downside: no explicit commit call. A bit "too clever".

* Nothing on destructor.

Easy to leave a tape in a broken state.

Differential Revision: D53311830

fbshipit-source-id: 88d59e95d39e9b42a5c7bfc6eac3650402b930c9

folly/container/tape.h

@@ -64,7 +64,8 @@ namespace folly {
  * `record_builder` interface.
  *
  * Existing `records` can be accessed by index.
- * Existing `records` cannot be mutated.
+ * Existing `records` cannot be mutated, except for the last record (see record
+ * builder).
  *
  * ## tape<tape>
  *
@@ -276,8 +277,13 @@ class tape {
 
   // record builder (constructing last record) -------
 
-  class scoped_record_builder;
-  [[nodiscard]] scoped_record_builder record_builder();
+  class record_builder;
+
+  // get a record builder.
+  // new_record_builder starts a builder for a new record.
+  // last_record_builder allows you to append/mutate the last record.
+  [[nodiscard]] record_builder new_record_builder();
+  [[nodiscard]] record_builder last_record_builder();
 
   // insert one record ----------
 
@@ -328,6 +334,7 @@ class tape {
   // erase -------
 
   void pop_back() noexcept {
+    assert(!empty());
     data_.resize(data_.size() - back().size());
     markers_.pop_back();
   }
@@ -369,13 +376,15 @@ class tape {
 
 // Provides a way to construct a last record similar
 // to how you would `std::vector`.
+// Typical workflow is you `push_back` a bunch of individual elements and then
+// `commit()`.
 template <FOLLY_TAPE_CONTAINER_REQUIRES Container>
-class tape<Container>::scoped_record_builder {
+class tape<Container>::record_builder {
  public:
-  scoped_record_builder(const scoped_record_builder&) = delete;
-  scoped_record_builder(scoped_record_builder&&) = delete;
-  scoped_record_builder& operator=(const scoped_record_builder&) = delete;
-  scoped_record_builder& operator=(scoped_record_builder&&) = delete;
+  record_builder(const record_builder&) = delete;
+  record_builder(record_builder&&) = delete;
+  record_builder& operator=(const record_builder&) = delete;
+  record_builder& operator=(record_builder&&) = delete;
 
   using iterator = typename container_type::iterator;
   using const_iterator = typename container_type::const_iterator;
@@ -386,6 +395,23 @@ class tape<Container>::scoped_record_builder {
   using difference_type =
       typename std::iterator_traits<iterator>::difference_type;
 
+  // mutators ---
+
+  void push_back(scalar_value_type x) { self_->data_.push_back(std::move(x)); }
+
+  template <typename... Args>
+  reference emplace_back(Args&&... args) {
+    self_->data_.emplace_back(std::forward<Args>(args)...);
+    // cannot rely on the container doing the right thing here.
+    return self_->data_.back();
+  }
+
+  // constructed record is added to the tape.
+  void commit() { self_->markers_.push_back(self_->data_.size()); }
+
+  // discards elements of the constructed record. (automatic on destruction)
+  void abort() { self_->data_.resize(self_->markers_.back()); }
+
   // iterators -----
 
   [[nodiscard]] iterator begin() noexcept {
@@ -443,33 +469,26 @@ class tape<Container>::scoped_record_builder {
   [[nodiscard]] reference back() { return self_->data_.back(); }
   [[nodiscard]] const_reference back() const { return self_->data_.back(); }
 
-  // mutators ---
-
-  void push_back(scalar_value_type x) { self_->data_.push_back(std::move(x)); }
-
-  template <typename... Args>
-  reference emplace_back(Args&&... args) {
-    self_->data_.emplace_back(std::forward<Args>(args)...);
-    // cannot rely on the container doing the right thing here.
-    return self_->data_.back();
-  }
-
-  ~scoped_record_builder() noexcept {
-    // we assume that allocations never fail
-    self_->markers_.push_back(self_->data_.size());
-  }
+  ~record_builder() noexcept { abort(); }
 
  private:
   friend class tape;
 
-  explicit scoped_record_builder(tape& self) : self_(&self) {}
+  explicit record_builder(tape& self) : self_(&self) {}
 
   tape* self_;
 };
 
 template <FOLLY_TAPE_CONTAINER_REQUIRES Container>
-auto tape<Container>::record_builder() -> scoped_record_builder {
-  return scoped_record_builder{*this};
+auto tape<Container>::new_record_builder() -> record_builder {
+  return record_builder{*this};
+}
+
+template <FOLLY_TAPE_CONTAINER_REQUIRES Container>
+auto tape<Container>::last_record_builder() -> record_builder {
+  assert(!empty());
+  markers_.pop_back();
+  return new_record_builder();
 }
 
 // tape methods -----

folly/container/test/tape_bench.cpp

@@ -109,11 +109,12 @@ void pushBackByOne(
 template <typename T>
 void pushBackByOne(
     folly::tape<std::vector<T>>& cont, const std::vector<std::vector<T>>& in) {
+  auto builder = cont.new_record_builder();
   for (const auto& v : in) {
-    auto builder = cont.record_builder();
     for (const auto& x : v) {
       builder.push_back(x);
     }
+    builder.commit();
   }
 }
 

folly/container/test/tape_test.cpp

@@ -248,7 +248,7 @@ TEST(Tape, RecordBuilder) {
   folly::string_tape st;
 
   {
-    auto builder = st.record_builder();
+    auto builder = st.new_record_builder();
     ASSERT_TRUE(builder.empty());
     ASSERT_EQ(0U, builder.size());
     builder.push_back('a');
@@ -262,27 +262,86 @@ TEST(Tape, RecordBuilder) {
     ASSERT_EQ(std::string_view(builder.begin(), builder.end()), "ab");
     ASSERT_EQ(std::string_view(builder.cbegin(), builder.cend()), "ab");
     *builder.begin() = 'c';
+    builder.commit();
   }
   {
-    auto builder = st.record_builder();
+    auto builder = st.new_record_builder();
     ASSERT_EQ(builder.emplace_back('d'), 'd');
     builder[0] = 'e';
     ASSERT_EQ(std::as_const(builder)[0], 'e');
+    builder.commit();
   }
   {
-    auto builder = st.record_builder();
+    auto builder = st.new_record_builder();
     builder.emplace_back(); // test emplace with default constructor
     builder.at(0) = 'b';
     ASSERT_EQ(std::as_const(builder).at(0), 'b');
     builder[0] = 'a';
     ASSERT_EQ(std::as_const(builder)[0], 'a');
+    builder.commit();
   }
   {
-    auto builder = st.record_builder();
+    auto builder = st.new_record_builder();
     constexpr std::string_view s = "abc";
     std::copy(s.begin(), s.end(), builder.back_inserter());
+    builder.commit();
+  }
+  {
+    auto builder = st.new_record_builder();
+    builder.push_back('0');
+    builder.push_back('1');
+    // Default abort - not committed
+  }
+  {
+    // calling abort
+    auto builder = st.new_record_builder();
+    ASSERT_EQ(0U, builder.size());
+    builder.push_back('a');
+    builder.push_back('b');
+    ASSERT_EQ(2U, builder.size());
+    builder.abort();
+    ASSERT_EQ(0U, builder.size());
+    builder.push_back('a');
+    builder.push_back('b');
+    builder.commit();
+  }
+  {
+    // amending record
+    ASSERT_EQ(st.back(), "ab");
+    auto builder = st.last_record_builder();
+    builder.push_back('c');
+    builder[0] = '0';
+    builder.commit();
+    ASSERT_EQ(st.back(), "0bc");
+  }
+  {
+    // not committing last record
+    ASSERT_EQ(st.size(), 5);
+
+    {
+      auto builder = st.new_record_builder();
+      builder.push_back('a');
+      builder.push_back('b');
+      builder.commit();
+    }
+
+    ASSERT_EQ(st.size(), 6);
+    (void)st.last_record_builder(); // destructor will auto abort.
+
+    ASSERT_EQ(st.size(), 5);
   }
 
+  ASSERT_EQ("cb", st[0]);
+  ASSERT_EQ("e", st[1]);
+  ASSERT_EQ("a", st[2]);
+  ASSERT_EQ("abc", st[3]);
+  ASSERT_EQ("0bc", st[4]);
+  ASSERT_EQ(10U, st.size_flat());
+  ASSERT_EQ(5U, st.size());
+
+  // checking that pop back is still ok.
+
+  st.pop_back();
   ASSERT_EQ("cb", st[0]);
   ASSERT_EQ("e", st[1]);
   ASSERT_EQ("a", st[2]);
@@ -515,18 +574,15 @@ TEST(Tape, Iteration) {
 TEST(Tape, TapeOfTapes) {
   folly::tape<folly::string_tape> strings;
 
-  {
-    auto builder = strings.record_builder();
-    builder.push_back("ab");
-    builder.push_back("abc");
-  }
+  auto builder = strings.new_record_builder();
+  builder.push_back("ab");
+  builder.push_back("abc");
+  builder.commit();
 
-  {
-    auto builder = strings.record_builder();
-    builder.push_back("bc");
-    builder.push_back("bcd");
-    builder.push_back("bcde");
-  }
+  builder.push_back("bc");
+  builder.push_back("bcd");
+  builder.push_back("bcde");
+  builder.commit();
 
   ASSERT_EQ(strings.size(), 2);
   ASSERT_THAT(strings[0], testing::ElementsAre("ab", "abc"));