Change the enumerate() example to bind the proxy by reference

Summary:
When compiling without optimizations binding the proxy by
reference is slightly faster (no differences in opt mode), so change
the documentation to recommend this syntax.

The proxy can still be bound by `auto`, `const auto`, and `const
auto&`, in all case behaving as expected and with no overhead in opt
mode. Added a test to make sure these all work.

Reviewed By: yfeldblum, luciang

Differential Revision: D6688958

fbshipit-source-id: 7c6b460a01708786bda7614546fa2e1667f27299

diff --git a/folly/container/Enumerate.h b/folly/container/Enumerate.h
index 79765e5163b67f890cab9f4fd5bed217e278763f..ff0404720b8f41ff04867d7e1c08712413ab94eb 100644 (file)
--- a/folly/container/Enumerate.h
+++ b/folly/container/Enumerate.h
@@ -29,7 +29,7 @@
  *
  * For example:
  *
- * for (auto it : folly::enumerate(vec)) {
+ * for (auto&& it : folly::enumerate(vec)) {
  *   // *it is a reference to the current element. Const if vec is const.
  *   // it->member can be used as well.
  *   // it.index contains the iteration count.
@@ -37,7 +37,7 @@
  *
  * If the iteration variable is const, the reference is too.
  *
- * for (const auto it : folly::enumerate(vec)) {
+ * for (const auto&& it : folly::enumerate(vec)) {
  *   // *it is always a const reference.
  * }
  *
@@ -125,12 +125,14 @@ class Enumerator {
   }
 
   template <typename OtherIterator>
-  FOLLY_ALWAYS_INLINE bool operator==(const Enumerator<OtherIterator>& rhs) {
+  FOLLY_ALWAYS_INLINE bool operator==(
+      const Enumerator<OtherIterator>& rhs) const {
     return it_ == rhs.it_;
   }
 
   template <typename OtherIterator>
-  FOLLY_ALWAYS_INLINE bool operator!=(const Enumerator<OtherIterator>& rhs) {
+  FOLLY_ALWAYS_INLINE bool operator!=(
+      const Enumerator<OtherIterator>& rhs) const {
     return !(it_ == rhs.it_);
   }
 

diff --git a/folly/container/test/EnumerateTest.cpp b/folly/container/test/EnumerateTest.cpp
index c96abca0b83742785561c632b6667ab033e83881..92de5fae20c6ac35e18baa02364e954cd367442c 100644 (file)
--- a/folly/container/test/EnumerateTest.cpp
+++ b/folly/container/test/EnumerateTest.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright 2017 Facebook, Inc.
+ * Copyright 2017-present Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -22,38 +22,6 @@
 #include <folly/container/Enumerate.h>
 #include <folly/portability/GTest.h>
 
-TEST(Enumerate, Basic) {
-  std::vector<std::string> v = {"abc", "a", "ab"};
-  size_t i = 0;
-  for (auto it : folly::enumerate(v)) {
-    EXPECT_EQ(it.index, i);
-    EXPECT_EQ(*it, v[i]);
-    EXPECT_EQ(it->size(), v[i].size());
-
-    // Test mutability.
-    std::string newValue = "x";
-    *it = newValue;
-    EXPECT_EQ(newValue, v[i]);
-
-    ++i;
-  }
-
-  EXPECT_EQ(i, v.size());
-}
-
-TEST(Enumerate, Temporary) {
-  std::vector<std::string> v = {"abc", "a", "ab"};
-  size_t i = 0;
-  for (auto it : folly::enumerate(decltype(v)(v))) { // Copy v.
-    EXPECT_EQ(it.index, i);
-    EXPECT_EQ(*it, v[i]);
-    EXPECT_EQ(it->size(), v[i].size());
-    ++i;
-  }
-
-  EXPECT_EQ(i, v.size());
-};
-
 namespace {
 
 template <class T>
@@ -67,12 +35,57 @@ struct IsConstReference<const T&> {
 
 } // namespace
 
-TEST(Enumerate, BasicConstArg) {
-  const std::vector<std::string> v = {"abc", "a", "ab"};
+#define ENUMERATE_TEST_BASIC(DECL, NAME)             \
+  TEST(Enumerate, NAME) {                            \
+    std::vector<std::string> v = {"abc", "a", "ab"}; \
+    size_t i = 0;                                    \
+    for (DECL it : folly::enumerate(v)) {            \
+      EXPECT_EQ(it.index, i);                        \
+      EXPECT_EQ(*it, v[i]);                          \
+      EXPECT_EQ(it->size(), v[i].size());            \
+                                                     \
+      /* Test mutability. */                         \
+      std::string newValue = "x";                    \
+      *it = newValue;                                \
+      EXPECT_EQ(newValue, v[i]);                     \
+                                                     \
+      ++i;                                           \
+    }                                                \
+                                                     \
+    EXPECT_EQ(i, v.size());                          \
+  }
+
+ENUMERATE_TEST_BASIC(auto, Basic)
+ENUMERATE_TEST_BASIC(auto&&, BasicRRef)
+
+#undef ENUMERATE_TEST_BASIC
+
+#define ENUMERATE_TEST_BASIC_CONST(DECL, NAME)                          \
+  TEST(Enumerate, NAME) {                                               \
+    std::vector<std::string> v = {"abc", "a", "ab"};                    \
+    size_t i = 0;                                                       \
+    for (DECL it : folly::enumerate(v)) {                               \
+      static_assert(                                                    \
+          IsConstReference<decltype(*it)>::value, "Const enumeration"); \
+      EXPECT_EQ(it.index, i);                                           \
+      EXPECT_EQ(*it, v[i]);                                             \
+      EXPECT_EQ(it->size(), v[i].size());                               \
+      ++i;                                                              \
+    }                                                                   \
+                                                                        \
+    EXPECT_EQ(i, v.size());                                             \
+  }
+
+ENUMERATE_TEST_BASIC_CONST(const auto, BasicConst)
+ENUMERATE_TEST_BASIC_CONST(const auto&, BasicConstRef)
+ENUMERATE_TEST_BASIC_CONST(const auto&&, BasicConstRRef)
+
+#undef ENUMERATE_TEST_BASIC_CONST
+
+TEST(Enumerate, Temporary) {
+  std::vector<std::string> v = {"abc", "a", "ab"};
   size_t i = 0;
-  for (auto it : folly::enumerate(v)) {
-    static_assert(
-        IsConstReference<decltype(*it)>::value, "Enumerating a const vector");
+  for (auto&& it : folly::enumerate(decltype(v)(v))) { // Copy v.
     EXPECT_EQ(it.index, i);
     EXPECT_EQ(*it, v[i]);
     EXPECT_EQ(it->size(), v[i].size());
@@ -80,13 +93,14 @@ TEST(Enumerate, BasicConstArg) {
   }
 
   EXPECT_EQ(i, v.size());
-}
+};
 
-TEST(Enumerate, BasicConstEnumerate) {
-  std::vector<std::string> v = {"abc", "a", "ab"};
+TEST(Enumerate, BasicConstArg) {
+  const std::vector<std::string> v = {"abc", "a", "ab"};
   size_t i = 0;
-  for (const auto it : folly::enumerate(v)) {
-    static_assert(IsConstReference<decltype(*it)>::value, "Const enumeration");
+  for (auto&& it : folly::enumerate(v)) {
+    static_assert(
+        IsConstReference<decltype(*it)>::value, "Enumerating a const vector");
     EXPECT_EQ(it.index, i);
     EXPECT_EQ(*it, v[i]);
     EXPECT_EQ(it->size(), v[i].size());
@@ -99,7 +113,7 @@ TEST(Enumerate, BasicConstEnumerate) {
 TEST(Enumerate, TemporaryConstEnumerate) {
   std::vector<std::string> v = {"abc", "a", "ab"};
   size_t i = 0;
-  for (const auto it : folly::enumerate(decltype(v)(v))) { // Copy v.
+  for (const auto&& it : folly::enumerate(decltype(v)(v))) { // Copy v.
     static_assert(IsConstReference<decltype(*it)>::value, "Const enumeration");
     EXPECT_EQ(it.index, i);
     EXPECT_EQ(*it, v[i]);
@@ -113,7 +127,7 @@ TEST(Enumerate, TemporaryConstEnumerate) {
 TEST(Enumerate, RangeSupport) {
   std::vector<std::string> v = {"abc", "a", "ab"};
   size_t i = 0;
-  for (const auto it : folly::enumerate(folly::range(v))) {
+  for (const auto&& it : folly::enumerate(folly::range(v))) {
     EXPECT_EQ(it.index, i);
     EXPECT_EQ(*it, v[i]);
     EXPECT_EQ(it->size(), v[i].size());
@@ -125,7 +139,7 @@ TEST(Enumerate, RangeSupport) {
 
 TEST(Enumerate, EmptyRange) {
   std::vector<std::string> v;
-  for (auto it : folly::enumerate(v)) {
+  for (auto&& it : folly::enumerate(v)) {
     (void)it; // Silence warnings.
     ADD_FAILURE();
   }
@@ -155,13 +169,13 @@ TEST(Enumerate, Cpp17Support) {
   std::array<char, 5> test = {"test"};
   // Can't use range based for loop until C++17, so test manually
   // Equivalent to:
-  // for (const auto it : folly::enumerate(CStringRange{test.data()})) { ... }
+  // for (const auto&& it : folly::enumerate(CStringRange{test.data()})) { ... }
   {
     auto&& enumerate = folly::enumerate(CStringRange{test.data()});
     auto begin = enumerate.begin();
     auto end = enumerate.end();
     for (; begin != end; ++begin) {
-      const auto it = *begin;
+      const auto&& it = *begin;
 
       ASSERT_LT(it.index, test.size());
       EXPECT_EQ(*it, test[it.index]);

diff --git a/folly/container/test/ForeachBenchmark.cpp b/folly/container/test/ForeachBenchmark.cpp
index a3e810eb5c905135ef173c5263e27e67deb98434..083cba5fc864494b67070e7316ee28e0dff64c38 100644 (file)
--- a/folly/container/test/ForeachBenchmark.cpp
+++ b/folly/container/test/ForeachBenchmark.cpp
@@ -389,7 +389,7 @@ BENCHMARK(CharVecForRangeEnumerate, iters) {
     setupCharVecBenchmark(iters);
   }
   size_t sum = 0;
-  for (auto it : enumerate(vec_char)) {
+  for (auto&& it : enumerate(vec_char)) {
     sum += *it * it.index;
   }
   doNotOptimizeAway(sum);