From 587e4b4d914c477b79120fb6559571d2dc5d040d Mon Sep 17 00:00:00 2001
From: Delyan Kratunov <delyank@fb.com>
Date: Thu, 19 Nov 2015 16:25:09 -0800
Subject: [PATCH] Expose move result from LockFreeRingBuffer::Cursor

Summary: Without feedback that a `moveForward` or `moveBackward` did nothing, a user
cannot implement this idiom safely:

```
while(rb.tryRead(entry, cursor)) {
  doSomething(entry);
  cursor.moveBackward();
}
```

In the case where the ring buffer has not overflowed and slot 0 is still
available, the reader will get stuck at it (read it continuously) until
the buffer overflows.

This diff allows the above example to become:

```
while(rb.tryRead(entry, cursor)) {
  doSomething(etnry);
  if (!cursor.moveBackward()) {
    break;
  }
}
```

Reviewed By: bryceredd

Differential Revision: D2674975

fb-gh-sync-id: a0c5857daf186ef19e203f90acc2145590f85c3b
---
 folly/experimental/LockFreeRingBuffer.h       | 12 ++++++++--
 .../test/LockFreeRingBufferTest.cpp           | 24 +++++++++++++++----
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/folly/experimental/LockFreeRingBuffer.h b/folly/experimental/LockFreeRingBuffer.h
index 0a2d10f3..f8c76e41 100644
--- a/folly/experimental/LockFreeRingBuffer.h
+++ b/folly/experimental/LockFreeRingBuffer.h
@@ -70,16 +70,24 @@ public:
   struct Cursor {
     explicit Cursor(uint64_t initialTicket) noexcept : ticket(initialTicket) {}
 
-    void moveForward(uint64_t steps = 1) noexcept {
+    /// Returns true if this cursor now points to a different
+    /// write, false otherwise.
+    bool moveForward(uint64_t steps = 1) noexcept {
+      uint64_t prevTicket = ticket;
       ticket += steps;
+      return prevTicket != ticket;
     }
 
-    void moveBackward(uint64_t steps = 1) noexcept {
+    /// Returns true if this cursor now points to a previous
+    /// write, false otherwise.
+    bool moveBackward(uint64_t steps = 1) noexcept {
+      uint64_t prevTicket = ticket;
       if (steps > ticket) {
         ticket = 0;
       } else {
         ticket -= steps;
       }
+      return prevTicket != ticket;
     }
 
   protected: // for test visibility reasons
diff --git a/folly/experimental/test/LockFreeRingBufferTest.cpp b/folly/experimental/test/LockFreeRingBufferTest.cpp
index 4b2447d3..8c853026 100644
--- a/folly/experimental/test/LockFreeRingBufferTest.cpp
+++ b/folly/experimental/test/LockFreeRingBufferTest.cpp
@@ -95,18 +95,16 @@ TEST(LockFreeRingBuffer, readsCanBlock) {
 
 // expose the cursor raw value via a wrapper type
 template<typename T, template<typename> class Atom>
-uint64_t value(const typename LockFreeRingBuffer<T, Atom>::Cursor&& rbcursor) {
+uint64_t value(const typename LockFreeRingBuffer<T, Atom>::Cursor& rbcursor) {
   typedef typename LockFreeRingBuffer<T,Atom>::Cursor RBCursor;
 
-  RBCursor cursor = std::move(rbcursor);
-
   struct ExposedCursor : RBCursor {
     ExposedCursor(const RBCursor& cursor): RBCursor(cursor) {}
     uint64_t value(){
       return this->ticket;
     }
   };
-  return ExposedCursor(cursor).value();
+  return ExposedCursor(rbcursor).value();
 }
 
 template<template<typename> class Atom>
@@ -243,4 +241,22 @@ TEST(LockFreeRingBuffer, cursorFromWrites) {
   EXPECT_EQ(3, cursorValue(rb.writeAndGetCursor(val)));
 }
 
+TEST(LockFreeRingBuffer, moveBackwardsCanFail) {
+  const int capacity = 3;
+  LockFreeRingBuffer<int> rb(capacity);
+
+  // Workaround for template deduction failure
+  auto (&cursorValue)(value<int, std::atomic>);
+
+  int val = 0xfaceb00c;
+  rb.write(val);
+  rb.write(val);
+
+  auto cursor = rb.currentHead(); // points to 2
+  EXPECT_EQ(2, cursorValue(cursor));
+  EXPECT_TRUE(cursor.moveBackward());
+  EXPECT_TRUE(cursor.moveBackward()); // now at 0
+  EXPECT_FALSE(cursor.moveBackward()); // moving back does nothing
+}
+
 } // namespace folly
-- 
2.34.1