From 489eeb1c2459073d09151b359109369e3f395d06 Mon Sep 17 00:00:00 2001 From: Giuseppe Ottaviano Date: Tue, 9 Jan 2018 18:44:44 -0800 Subject: [PATCH] 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 --- folly/container/Enumerate.h | 10 +- folly/container/test/EnumerateTest.cpp | 110 ++++++++++++---------- folly/container/test/ForeachBenchmark.cpp | 2 +- 3 files changed, 69 insertions(+), 53 deletions(-) diff --git a/folly/container/Enumerate.h b/folly/container/Enumerate.h index 79765e51..ff040472 100644 --- 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 - FOLLY_ALWAYS_INLINE bool operator==(const Enumerator& rhs) { + FOLLY_ALWAYS_INLINE bool operator==( + const Enumerator& rhs) const { return it_ == rhs.it_; } template - FOLLY_ALWAYS_INLINE bool operator!=(const Enumerator& rhs) { + FOLLY_ALWAYS_INLINE bool operator!=( + const Enumerator& rhs) const { return !(it_ == rhs.it_); } diff --git a/folly/container/test/EnumerateTest.cpp b/folly/container/test/EnumerateTest.cpp index c96abca0..92de5fae 100644 --- 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 #include -TEST(Enumerate, Basic) { - std::vector 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 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 @@ -67,12 +35,57 @@ struct IsConstReference { } // namespace -TEST(Enumerate, BasicConstArg) { - const std::vector v = {"abc", "a", "ab"}; +#define ENUMERATE_TEST_BASIC(DECL, NAME) \ + TEST(Enumerate, NAME) { \ + std::vector 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 v = {"abc", "a", "ab"}; \ + size_t i = 0; \ + for (DECL it : folly::enumerate(v)) { \ + static_assert( \ + IsConstReference::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 v = {"abc", "a", "ab"}; size_t i = 0; - for (auto it : folly::enumerate(v)) { - static_assert( - IsConstReference::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 v = {"abc", "a", "ab"}; +TEST(Enumerate, BasicConstArg) { + const std::vector v = {"abc", "a", "ab"}; size_t i = 0; - for (const auto it : folly::enumerate(v)) { - static_assert(IsConstReference::value, "Const enumeration"); + for (auto&& it : folly::enumerate(v)) { + static_assert( + IsConstReference::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 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::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 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 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 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 a3e810eb..083cba5f 100644 --- 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); -- 2.34.1