-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[ADT] Add sum_of and product_of accumulate wrappers
#162129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Also extend the `accumulate` wrapper to accept a binary operator. The goal is to the most common usage of `llvm::accumulate` accross the codebase -- calculating either the sum of or the product of all values.
|
@llvm/pr-subscribers-llvm-adt Author: Jakub Kuderski (kuhar) ChangesAlso extend the The goal is to the most common usage of Full diff: https://github.com/llvm/llvm-project/pull/162129.diff 2 Files Affected:
diff --git a/llvm/include/llvm/ADT/STLExtras.h b/llvm/include/llvm/ADT/STLExtras.h
index 4a91b061dd3b7..5b20d6bd38262 100644
--- a/llvm/include/llvm/ADT/STLExtras.h
+++ b/llvm/include/llvm/ADT/STLExtras.h
@@ -1692,6 +1692,28 @@ template <typename R, typename E> auto accumulate(R &&Range, E &&Init) {
std::forward<E>(Init));
}
+/// Wrapper for std::accumulate with a binary operator.
+template <typename R, typename E, typename BinaryOp>
+auto accumulate(R &&Range, E &&Init, BinaryOp &&Op) {
+ return std::accumulate(adl_begin(Range), adl_end(Range),
+ std::forward<E>(Init), std::forward<BinaryOp>(Op));
+}
+
+/// Returns the sum of all values in `Range` with `Init` initial value.
+/// The default initial value is 0.
+template <typename R, typename E = detail::ValueOfRange<R>>
+auto sum_of(R &&Range, E Init = E{0}) {
+ return accumulate(std::forward<R>(Range), std::move(Init));
+}
+
+/// Returns the product of all values in `Range` with `Init` initial value.
+/// The default initial value is 1.
+template <typename R, typename E = detail::ValueOfRange<R>>
+auto product_of(R &&Range, E Init = E{1}) {
+ return accumulate(std::forward<R>(Range), std::move(Init),
+ std::multiplies<>{});
+}
+
/// Provide wrappers to std::for_each which take ranges instead of having to
/// pass begin/end explicitly.
template <typename R, typename UnaryFunction>
diff --git a/llvm/unittests/ADT/STLExtrasTest.cpp b/llvm/unittests/ADT/STLExtrasTest.cpp
index 5020acda95b0b..52fb423713a05 100644
--- a/llvm/unittests/ADT/STLExtrasTest.cpp
+++ b/llvm/unittests/ADT/STLExtrasTest.cpp
@@ -14,6 +14,7 @@
#include <array>
#include <climits>
#include <cstddef>
+#include <functional>
#include <initializer_list>
#include <iterator>
#include <list>
@@ -1658,6 +1659,44 @@ TEST(STLExtrasTest, Accumulate) {
EXPECT_EQ(accumulate(V1, 10), std::accumulate(V1.begin(), V1.end(), 10));
EXPECT_EQ(accumulate(drop_begin(V1), 7),
std::accumulate(V1.begin() + 1, V1.end(), 7));
+
+ EXPECT_EQ(accumulate(V1, 2, std::multiplies<>{}), 240);
+}
+
+TEST(STLExtrasTest, SumOf) {
+ EXPECT_EQ(sum_of(std::vector<int>()), 0);
+ EXPECT_EQ(sum_of(std::vector<int>(), 1), 1);
+ std::vector<int> V1 = {1, 2, 3, 4, 5};
+ static_assert(std::is_same_v<decltype(sum_of(V1)), int>);
+ static_assert(std::is_same_v<decltype(sum_of(V1, 1)), int>);
+ EXPECT_EQ(sum_of(V1), 15);
+ EXPECT_EQ(sum_of(V1, 1), 16);
+
+ std::vector<float> V2 = {1.0f, 2.0f, 4.0f};
+ static_assert(std::is_same_v<decltype(sum_of(V2)), float>);
+ static_assert(std::is_same_v<decltype(sum_of(V2), 1.0f), float>);
+ static_assert(std::is_same_v<decltype(sum_of(V2), 1.0), double>);
+ EXPECT_EQ(sum_of(V2), 7.0f);
+ EXPECT_EQ(sum_of(V2, 1.0f), 8.0f);
+}
+
+TEST(STLExtrasTest, ProductOf) {
+ EXPECT_EQ(product_of(std::vector<int>()), 1);
+ EXPECT_EQ(product_of(std::vector<int>(), 0), 0);
+ EXPECT_EQ(product_of(std::vector<int>(), 1), 1);
+ std::vector<int> V1 = {1, 2, 3, 4, 5};
+ static_assert(std::is_same_v<decltype(product_of(V1)), int>);
+ static_assert(std::is_same_v<decltype(product_of(V1, 1)), int>);
+ EXPECT_EQ(product_of(V1), 120);
+ EXPECT_EQ(product_of(V1, 1), 120);
+ EXPECT_EQ(product_of(V1, 2), 240);
+
+ std::vector<float> V2 = {1.0f, 2.0f, 4.0f};
+ static_assert(std::is_same_v<decltype(product_of(V2)), float>);
+ static_assert(std::is_same_v<decltype(product_of(V2), 1.0f), float>);
+ static_assert(std::is_same_v<decltype(product_of(V2), 1.0), double>);
+ EXPECT_EQ(product_of(V2), 8.0f);
+ EXPECT_EQ(product_of(V2, 4.0f), 32.0f);
}
struct Foo;
|
kazutakahirata
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
| /// Returns the sum of all values in `Range` with `Init` initial value. | ||
| /// The default initial value is 0. | ||
| template <typename R, typename E = detail::ValueOfRange<R>> | ||
| auto sum_of(R &&Range, E Init = E{0}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why not just sum and product?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do that but I didn't want to hijack shorter names that may already be used elsewhere... The worry is that we could have a name collision with any local function that named sum or product that takes a range.
mtrofin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
sum_of and product_of accumulate wrappers.sum_of and product_of accumulate wrappers
|
The windows failure is unrelated |
Adopt accumulate wrapers from llvm/llvm-project#162129. The main benefit is that these operate on ranges instead of iterators, and `product_of` does not require an initial value. Providing initial value is error prone because it determines the accumulator type, and it's easy to accidentally accumulate `int64_t` to a plain `int` by using `1` for the initial value.
Use wrappers around `std::accumulate` to make the code more concise and less bug-prone: #162129. With `std::accumulate`, it's the initial value that determines the accumulator type. `llvm::sum_of` and `llvm::product_of` pick the right accumulator type based on the range element type. Found some funny bugs like a local accumulate helper that calculated a sum with initial value of 1 -- we didn't hit the bug because the code was actually dead...
Use wrappers around `std::accumulate` to make the code more concise and less bug-prone: llvm/llvm-project#162129. With `std::accumulate`, it's the initial value that determines the accumulator type. `llvm::sum_of` and `llvm::product_of` pick the right accumulator type based on the range element type. Found some funny bugs like a local accumulate helper that calculated a sum with initial value of 1 -- we didn't hit the bug because the code was actually dead...
Adopt accumulate wrapers from llvm/llvm-project#162129. The main benefit is that these operate on ranges instead of iterators, and `product_of` does not require an initial value. Providing initial value is error prone because it determines the accumulator type, and it's easy to accidentally accumulate `int64_t` to a plain `int` by using `1` for the initial value.
Use wrappers around `std::accumulate` to make the code more concise and less bug-prone: llvm#162129. With `std::accumulate`, it's the initial value that determines the accumulator type. `llvm::sum_of` and `llvm::product_of` pick the right accumulator type based on the range element type. Found some funny bugs like a local accumulate helper that calculated a sum with initial value of 1 -- we didn't hit the bug because the code was actually dead...
Adopt accumulate wrappers from llvm/llvm-project#162129. The main benefit is that these operate on ranges instead of iterators, and `product_of` does not require an initial value. Providing initial value is error prone because it determines the accumulator type, and it's easy to accidentally accumulate `int64_t` to a plain `int` by using `1` for the initial value.
Use wrappers around `std::accumulate` to make the code more concise and less bug-prone: llvm#162129. With `std::accumulate`, it's the initial value that determines the accumulator type. `llvm::sum_of` and `llvm::product_of` pick the right accumulator type based on the range element type. Found some funny bugs like a local accumulate helper that calculated a sum with initial value of 1 -- we didn't hit the bug because the code was actually dead...
Also extend the `accumulate` wrapper to accept a binary operator. The goal is to the most common usage of `std::accumulate` across the codebase -- calculating either the sum of or the product of all values. (cherry picked from commit 454ef02)
Adopt accumulate wrappers from llvm/llvm-project#162129. The main benefit is that these operate on ranges instead of iterators, and `product_of` does not require an initial value. Providing initial value is error prone because it determines the accumulator type, and it's easy to accidentally accumulate `int64_t` to a plain `int` by using `1` for the initial value. Signed-off-by: Philipp <[email protected]>
Also extend the `accumulate` wrapper to accept a binary operator. The goal is to the most common usage of `std::accumulate` across the codebase -- calculating either the sum of or the product of all values. (cherry picked from commit 454ef02)
Also extend the
accumulatewrapper to accept a binary operator.The goal is to the most common usage of
std::accumulateacross the codebase -- calculating either the sum of or the product of all values.