Fix EliasFanoReader position() when past-the-end

Summary:
`EliasFanoReader::position()` used to return `size() - 1` both when
the reader is positioned on the last element, and after `next()` is
called after that (and it return `false`). Now in the latter case
`position()` returns `size()` (consistently with the usual behaviour
of past-the-end iterators).

Also fix the return type of `jumpTo`.

Test Plan:
fbconfig folly/experimental/test:eliasfano_test && fbmake runtests_opt

Reviewed By: philipp@fb.com

Subscribers: trunkagent, folly-diffs@, yfeldblum

FB internal diff: D1846275

Signature: t1:1846275:1423790264:151f5d2e1e09d4e24dfb758473dfb9cd52c070bd

diff --git a/folly/experimental/EliasFanoCoding.h b/folly/experimental/EliasFanoCoding.h
index 61335819cc15375da30b9d5839a6aedccc4ab870..7f83b0e8ab40d047a039e8749f1d6e9da9f8cc10 100644 (file)
--- a/folly/experimental/EliasFanoCoding.h
+++ b/folly/experimental/EliasFanoCoding.h
@@ -542,9 +542,8 @@ class EliasFanoReader : private boost::noncopyable {
   }
 
   bool next() {
-    if (UNLIKELY(progress_ == list_.size)) {
-      value_ = std::numeric_limits<ValueType>::max();
-      return false;
+    if (UNLIKELY(progress_ >= list_.size)) {
+      return setDone();
     }
     value_ = readLowerPart(progress_) |
              (upper_.next() << list_.numLowerBits);
@@ -567,9 +566,7 @@ class EliasFanoReader : private boost::noncopyable {
       return true;
     }
 
-    progress_ = list_.size;
-    value_ = std::numeric_limits<ValueType>::max();
-    return false;
+    return setDone();
   }
 
   bool skipTo(ValueType value) {
@@ -577,9 +574,7 @@ class EliasFanoReader : private boost::noncopyable {
     if (value <= value_) {
       return true;
     } else if (value > lastValue_) {
-      progress_ = list_.size;
-      value_ = std::numeric_limits<ValueType>::max();
-      return false;
+      return setDone();
     }
 
     size_t upperValue = (value >> list_.numLowerBits);
@@ -607,19 +602,15 @@ class EliasFanoReader : private boost::noncopyable {
       reset();
       return true;
     }
-    progress_ = list_.size;
-    value_ = std::numeric_limits<ValueType>::max();
-    return false;
+    return setDone();
   }
 
-  ValueType jumpTo(ValueType value) {
+  bool jumpTo(ValueType value) {
     if (value <= 0) {
       reset();
       return true;
     } else if (value > lastValue_) {
-      progress_ = list_.size;
-      value_ = std::numeric_limits<ValueType>::max();
-      return false;
+      return setDone();
     }
 
     upper_.jumpToNext(value >> list_.numLowerBits);
@@ -633,6 +624,12 @@ class EliasFanoReader : private boost::noncopyable {
   ValueType value() const { return value_; }
 
  private:
+  bool setDone() {
+    value_ = std::numeric_limits<ValueType>::max();
+    progress_ = list_.size + 1;
+    return false;
+  }
+
   ValueType readLowerPart(size_t i) const {
     DCHECK_LT(i, list_.size);
     const size_t pos = i * list_.numLowerBits;

diff --git a/folly/experimental/test/CodingTestUtils.h b/folly/experimental/test/CodingTestUtils.h
index 2d686b95a03494426b1dd135a33a5f8c61b293ab..1c53121fc845a5e360a3c4136bd8a9ae08d102c1 100644 (file)
--- a/folly/experimental/test/CodingTestUtils.h
+++ b/folly/experimental/test/CodingTestUtils.h
@@ -84,7 +84,7 @@ void testNext(const std::vector<uint32_t>& data, const List& list) {
   }
   EXPECT_FALSE(reader.next());
   EXPECT_EQ(reader.value(), std::numeric_limits<uint32_t>::max());
-  EXPECT_EQ(reader.position(), reader.size() - 1);
+  EXPECT_EQ(reader.position(), reader.size());
 }
 
 template <class Reader, class List>
@@ -100,7 +100,7 @@ void testSkip(const std::vector<uint32_t>& data, const List& list,
   }
   EXPECT_FALSE(reader.skip(skipStep));
   EXPECT_EQ(reader.value(), std::numeric_limits<uint32_t>::max());
-  EXPECT_EQ(reader.position(), reader.size() - 1);
+  EXPECT_EQ(reader.position(), reader.size());
   EXPECT_FALSE(reader.next());
 }
 
@@ -136,7 +136,7 @@ void testSkipTo(const std::vector<uint32_t>& data, const List& list,
     value = reader.value() + delta;
   }
   EXPECT_EQ(reader.value(), std::numeric_limits<uint32_t>::max());
-  EXPECT_EQ(reader.position(), reader.size() - 1);
+  EXPECT_EQ(reader.position(), reader.size());
   EXPECT_FALSE(reader.next());
 }
 
@@ -153,7 +153,7 @@ void testSkipTo(const std::vector<uint32_t>& data, const List& list) {
     Reader reader(list);
     EXPECT_FALSE(reader.skipTo(data.back() + 1));
     EXPECT_EQ(reader.value(), std::numeric_limits<uint32_t>::max());
-    EXPECT_EQ(reader.position(), reader.size() - 1);
+    EXPECT_EQ(reader.position(), reader.size());
     EXPECT_FALSE(reader.next());
   }
 }
@@ -180,7 +180,7 @@ void testJump(const std::vector<uint32_t>& data, const List& list) {
   }
   EXPECT_FALSE(reader.jump(data.size() + 1));
   EXPECT_EQ(reader.value(), std::numeric_limits<uint32_t>::max());
-  EXPECT_EQ(reader.position(), reader.size() - 1);
+  EXPECT_EQ(reader.position(), reader.size());
 }
 
 template <class Reader, class List>
@@ -217,7 +217,7 @@ void testJumpTo(const std::vector<uint32_t>& data, const List& list) {
 
   EXPECT_FALSE(reader.jumpTo(data.back() + 1));
   EXPECT_EQ(reader.value(), std::numeric_limits<uint32_t>::max());
-  EXPECT_EQ(reader.position(), reader.size() - 1);
+  EXPECT_EQ(reader.position(), reader.size());
 }
 
 template <class Reader, class Encoder>