Implements P1645: constexpr for <numeric> algorithms
Details
- Reviewers
ldionne - Group Reviewers
Restricted Project - Commits
- rG67c88e47bdba: [libc++] P1645 constexpr for <numeric>
rGeb9b063539c3: [libc++] P1645 constexpr for <numeric>
Diff Detail
Event Timeline
Or do you prefer small patches per function? (In that case I'll to update the status since it now expects one patch.)
One patch for all is fine. But it's a good idea to iterate on this one function for now until we are good to go, and then you can do the other ones. This will avoid back and forth.
libcxx/include/numeric | ||
---|---|---|
20 | Please add // constexpr since C++20 to note that the API is only constexpr in C++20. | |
libcxx/test/std/numerics/numeric.ops/accumulate/accumulate.pass.cpp | ||
76 | The tests for runtime and constexpr should be the same. Can you please take a look at libcxx/test/std/utilities/memory/specialized.algorithms/specialized.destroy/destroy_n.pass.cpp to see how we do it elsewhere? |
Thanks for the feedback. Then I'll make one patch for the paper once the first function is approved.
libcxx/test/std/numerics/numeric.ops/accumulate/accumulate.pass.cpp | ||
---|---|---|
76 | That's a nice approach and indeed looks better than my approach. |
I had another look at the header and noticed something odd for all overloads of inclusive_scan and transform_inclusive_scan and their signatures are odd
template <class _InputIterator, class _OutputIterator, class _Tp, class _BinaryOp> _OutputIterator inclusive_scan(_InputIterator __first, _InputIterator __last, _OutputIterator __result, _BinaryOp __b, _Tp __init)
Instead of the expected
template <class _InputIterator, class _OutputIterator, class _Tp, class _BinaryOp> inline _LIBCPP_INLINE_VISIBILITY _OutputIterator inclusive_scan(_InputIterator __first, _InputIterator __last, _OutputIterator __result, _BinaryOp __b, _Tp __init)
Is there a reason why their signatures differ or is it an oversight?
Shall I fix them in this patch?
libcxx/test/std/numerics/numeric.ops/accumulate/accumulate.pass.cpp | ||
---|---|---|
10–11 | I reverted the original change and redid the changes. I just noticed I forgot to re-add the // Became constexpr in C++20 comment here. |
I believe those are oversights, and the blame seems to confirm this. Good catch, and please fix them in this patch. You don't need inline though, just _LIBCPP_INLINE_VISIBILITY. These functions are already inline (i.e. linkonce-ODR) because they are templates.
On a different note, I would like to mention that I really like your approach of making sure everything's fine with a single function, and then going for the whole thing. I feel like this is a productive way to proceed, and I appreciate that.
Your approach LGTM, feel free to go ahead with the full thing!
Added an updated test for libcxx/test/std/numerics/numeric.ops/exclusive.scan/exclusive_scan.pass.cpp.
Since this tests uses std::vector which can't be used as constexpr in libc++ at the moment I took a different approach. Several other tests have the same issue. @ldionne can you have a look only at this test before I proceed?
Thanks will do.
On a different note, I would like to mention that I really like your approach of making sure everything's fine with a single function, and then going for the whole thing. I feel like this is a productive way to proceed, and I appreciate that.
Thanks. I also prefer this way since it saves everybody time!
libcxx/test/std/numerics/numeric.ops/exclusive.scan/exclusive_scan.pass.cpp | ||
---|---|---|
39 ↗ | (On Diff #306144) | LGTM, but could you add a FIXME to remind us to go back and fix those once constexpr std::vector is finished? If you keep the FIXME exactly the same in all places where you use this pattern, we'll be able to search for it and easily replace all locations where we used the workaround. |
This should complete the implementation of P1645
Additional changes
- Since constexpr vector isn't available some test have a workaround to be tested without this feature. The changes are marked with a fixme to be removed once the support is available.
- Enables additional unit tests in libcxx/test/std/numerics/numeric.ops/inclusive.scan/inclusive_scan_op.pass.cpp. These were disabled when added, but that seems an oversight.
- Changes the changed numeric functions to all use _LIBCPP_INLINE_VISIBILITY and removed the unneeded inline.
This LGTM, but I'd really like to see CI running. I added the LLVM monorepo to the revision -- can you try rebasing onto master just to see if it'll trigger CI?
Disable the unit tests on clang-8, they fail and we don't support these old compilers for libc++.
Switch to the new status page
Adjusted the unit tests modified in D61170 to work in constexpr evaluation. Since constexpr string isn't supported yet the std::string tests are disabled in constant evaluation context.
Please add // constexpr since C++20 to note that the API is only constexpr in C++20.