[libc++] Implement ranges::iota #68494

jamesETsmith · 2023-10-07T18:51:40Z

Overview

As a disclaimer, this is my first PR to LLVM and while I've tried to ensure I've followed the LLVM and libc++ contributing guidelines, there's probably a good chance I missed something. If I have, just let me know and I'll try to correct it as soon as I can.

This PR implements std::ranges::iota and std::ranges::out_value_result outlined in P2440r1.

As outlined in the paper above, I've:

Implemented out_value_result and added to <algorithm>
Added out_value_result, iota_result, and two overloads of iota to std::ranges in <numeric>
Updated the version macro __cpp_lib_ranges_iota in <version>

I've also added tests for ranges::iota and ranges::out_value_result. Lastly, I added those structs to the appropriate module files.

Partially implements #105184

EDIT: Forgot to mention in the original post, thanks to @hawkinsw for taking a look at a preliminary version of this PR!

TODOs

Updating the range status doc
Ensure all comments from https://reviews.llvm.org/D121436 are addressed here
EDIT (I'll do this in a separate PR). ~~I'm open to implementing the rest of P2440r1 (ranges::shift_left and ranges::shift_right) if that's ok, I just wanted to get feedback on ranges::iota first~~
I've been having trouble building the modules locally and want to make sure that's working properly

Closes: #134060

…es_iota

…ue_result

…nd out_value_result. Updating Cxx23Papers.csv too.

…nd adding testing for more iterators

…cks happy

…es_iota

philnik777

I haven't looked at the tests yet. The implementation itself looks mostly good.

philnik777 · 2023-10-07T21:30:18Z

libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp

@@ -0,0 +1,124 @@
+//===----------------------------------------------------------------------===//


I've started working on this in https://reviews.llvm.org/D121436 (and didn't really work on it because nobody knew what it was for). You should probably make sure that any comments there are addressed here and add any additional tests from there.

libcxx/include/__numeric/ranges_iota.h

philnik777 · 2023-10-07T21:33:10Z

libcxx/include/__numeric/ranges_iota.h

+struct __iota_fn {
+  template <input_or_output_iterator _Out, sentinel_for<_Out> _Sent, weakly_incrementable _Tp>
+    requires indirectly_writable<_Out, const _Tp&>
+  constexpr iota_result<_Out, _Tp> operator()(_Out __first, _Sent __last, _Tp __value) const {


Suggested change

constexpr iota_result<_Out, _Tp> operator()(_Out __first, _Sent __last, _Tp __value) const {

static constexpr iota_result<_Out, _Tp> operator()(_Out __first, _Sent __last, _Tp __value) {

same below

@philnik777, I'm happy to make these changes, but could you explain why static is more appropriate than const here?

As an alternative, after I've added the helper function, I could make that static and leave the public functions as const like it says in the original paper. I stumbled on this strategy (it's used by ranges::for_each and ranges::generate at least) while looking at how other libcxx helper functions were structured.

EDIT: I realized it would probably be easier to just implement this second strategy (in a455f42) and see what you think.

static operator() has been introduced in C++23, so we can't (currently) use it for C++20 algorithms. Otherwise we would also use it there. The simple reason to use it is that we don't care about the object and the static version has slightly better code gen (https://godbolt.org/z/8E1f77q6f).

Ah I see, I wasn't aware that static operator() was introduced in C++23. That clears things up, thanks for the example.

libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp

libcxx/test/std/numerics/numeric.ops/numeric.iota/ranges.iota.pass.cpp

libcxx/test/std/algorithms/algorithms.results/out_value_result.pass.cpp

libcxx/docs/Status/Cxx23Papers.csv

philnik777 · 2023-10-07T21:39:47Z

libcxx/include/__numeric/ranges_iota.h

+
+  template <weakly_incrementable _Tp, ranges::output_range<const _Tp&> _Range>
+  constexpr iota_result<ranges::borrowed_iterator_t<_Range>, _Tp> operator()(_Range&& __r, _Tp __value) const {
+    return (*this)(ranges::begin(__r), ranges::end(__r), std::move(__value));


You'll probably run into problems with this when updating the robust_against_* tests. You should move the algorithm itself into a helper.

I don't see why this would cause problems?

Sorry I've been slow on this. @philnik777 do you have any suggestions/comments about this?

We have "robust" tests to make sure we're not copying or moving comparators and projections, but I don't think we do that for "values" as is the case here. Whether it's an omission or not is a separate question, but as it stands, I think the original form of this code (forwarding directly from the ranges overload to the iterator overload) should work -- can you please check that, @jamesETsmith?

I thought I had rechecked that the original implementation had caused problems with the robust_against_* tests, but after checking again today, it doesn't seem to cause any problems.

As a result, I've reverted to the original strategy at @cjdb and @var-const's suggestion.

jamesETsmith · 2023-10-10T10:08:26Z

Thanks for taking a look @philnik777! I'll try to address your comments in the next couple of days and let you know if I have any questions.

jamesETsmith · 2023-10-24T20:45:01Z

Sorry for the delay, just circling back to this. @philnik777 should I update the robust_against_* tests in both test/std/algorithms and test/libcxx/algorithms/?

Probably a simple follow-up question, but you're interested in adding ranges::iota to the ranges_robust_against_* right? Or should it be added to the robust_against_* tests?

…tion and updating docs.

philnik777 · 2023-10-25T09:11:07Z

Yes, it should be the ranges_robust_against_* tests in both test/libcxx and test/std.

…t_* tests (proxy iterators is failing currently).

…icts in docs

github-actions · 2023-10-26T21:09:35Z

✅ With the latest revision this PR passed the C/C++ code formatter.

jamesETsmith · 2023-10-26T21:31:03Z

@philnik777, I've added tests for ranges::iota to several of the ranges_robust_against_* tests where I thought it was appropriate. Below is a breakdown (mostly for my own tracking) of what I've done and what I'm still sorting through (ranges_robust_against_proxy_iterators). If you see anything that I missed or got wrong, just let me know.

libcxx/test/std/algorithms
- libcxx/test/std/algorithms/ranges_result_alias_declarations.compile.pass.cpp
- libcxx/test/std/algorithms/ranges_robust_against_dangling.pass.cpp
- libcxx/test/std/algorithms/ranges_robust_against_differing_projections.pass.cpp
  - I don't think we need this one, since we don't use projections
- libcxx/test/std/algorithms/ranges_robust_against_nonbool_predicates.pass.cpp
  - I don't think we need this one since we don't use predicates
- libcxx/test/std/algorithms/ranges_robust_against_omitting_invoke.pass.cpp
  - I don't think we need this one because we don't use invoke
- libcxx/test/std/algorithms/ranges_robust_against_proxy_iterators.pass.cpp
  - This one is giving me trouble bc because 'Proxy<int &>' does not satisfy 'weakly_incrementable'
libcxx/test/libcxx/
- libcxx/test/libcxx/algorithms/ranges_robust_against_copying_comparators.pass.cpp
  - I don't think we need this since we don't use comparators
- libcxx/test/libcxx/algorithms/ranges_robust_against_copying_projections.pass.cpp
  - I don't think we need this since we don't use projections

…result tests from https://reviews.llvm.org/D121436

philnik777 · 2023-10-27T09:04:56Z

Why does the proxy not satisfy weakly_incrementable? Is it maybe just missing some member functions? Otherwise the list looks reasonable.

jamesETsmith · 2023-10-27T13:35:29Z

@philnik777, I think you're right that Proxy is missing a few member functions (and type aliases). Here's a minimal example of my additions to it: https://godbolt.org/z/8T9eqjjEj. If that seems reasonable, I'll update test_iterators.h.

philnik777 · 2023-10-27T13:40:31Z

You should make the operators conditionally available based on whether the underlying type has them. The rest looks good.

jamesETsmith · 2023-10-27T19:30:48Z

Making the operations conditionals was pretty straightforward, but using difference_type = std::iter_difference_t<T> is causing problems because it requires that all T types implement T::difference_type which I think is too strong a requirement. For example, this will cause Proxy<MoveOnly> to crash because MoveOnly doesn't implement difference_type.

My gut reaction was to try and make that alias require a concept, but the more I try to get something like a "conditional using-declaration" to work the more it feels like that isn't allowed and I'm thinking about this the wrong way. Do you have any suggestions @philnik777?

Here's an updated minimal example showing the crash that I'm talking about: https://godbolt.org/z/KocGjhPhz

philnik777 · 2023-10-27T20:37:51Z

You can add a base class like this: https://godbolt.org/z/ccG7jcqee

jamesETsmith · 2023-10-27T21:08:30Z

That's much cleaner than what I was coming up with, thanks! I'll give that a try

strega-nil · 2024-07-22T20:33:01Z

@strega-nil, thanks for taking a look. You're welcome to merge it if you like. Do you want me to fix these merge conflicts or were you planning to do that? I'm happy to fix them, but don't want to duplicate that work if you've already started it locally.

I have not started it, it'd be great if you could do a merge commit 😄

jamesETsmith · 2024-07-22T20:59:24Z

@strega-nil, ok I'll do that now

…esolving merge conflicts

jamesETsmith · 2024-07-23T12:55:54Z

@strega-nil, I think this is good to go from my end unless people have further comments.

… version.version.compile tests rather than generating it with the python script so the formatting was off

jamesETsmith · 2024-07-24T16:20:08Z

I forgot (again) to update libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp using the script so the formatting of that file was incorrect. Sorry about that

strega-nil · 2024-07-24T16:48:36Z

I'll take a look at this tomorrow 😃

strega-nil

This looks great! Just some minor nits, but other than that almost ready to merge 😄

libcxx/docs/Status/RangesAlgorithms.csv

strega-nil · 2024-07-25T16:42:05Z

libcxx/docs/Status/RangesMajorFeatures.csv

@@ -2,4 +2,5 @@ Standard,Name,Assignee,CL,Status
 C++23,`ranges::to <https://wg21.link/P1206R7>`_,Konstantin Varlamov,`D142335 <https://reviews.llvm.org/D142335>`_,Complete
 C++23,`Pipe support for user-defined range adaptors <https://wg21.link/P2387R3>`_,Unassigned,No patch yet,Not started
 C++23,`Formatting Ranges <https://wg21.link/P2286R8>`_,Mark de Wever,Various,Complete
+C++23, `ranges::iota <https://wg21.link/P2440R1>`_, James E T Smith, `PR68494 <https://github.com/llvm/llvm-project/pull/68494>`_, In Progress


I don't believe I'd call this a "major ranges feature"; this is an important ranges feature, but isn't quite as large as the other things on this list; I'd remove this change.

libcxx/include/__algorithm/out_value_result.h

libcxx/include/algorithm

libcxx/include/module.modulemap

libcxx/test/std/algorithms/ranges_result_alias_declarations.compile.pass.cpp

strega-nil · 2024-07-25T17:02:27Z

libcxx/test/std/algorithms/ranges_robust_against_proxy_iterators.pass.cpp

@@ -100,6 +101,11 @@ constexpr void run_tests() {
  test(std::ranges::search, in, in2);
  test(std::ranges::search_n, in, count, x);
  test(std::ranges::find_end, in, in2);
+#if TEST_STD_VER >= 23
+  if constexpr (std::copyable<T>) {


We could also do if constexpr (weakly_incrementable<T>) { test(std::ranges::iota, in, T{}); }?

libcxx/test/support/test_iterators.h

libcxx/include/__numeric/ranges_iota.h

@strega-nil

…rom @strega-nil

jamesETsmith · 2024-07-26T18:35:49Z

@strega-nil, thanks again for looking this over. I hope I've addressed all your comments with b4008ad (with the exception of this one, which is I'm waiting on until we reach a resolution)

strega-nil

Thanks for finishing this up! Now we just need one more reviewer, and you can merge 😄

jamesETsmith · 2024-07-29T13:46:26Z

Thanks again @strega-nil!

@var-const, @cjdb, and @philnik777 could you take another look at this PR and let me know if I've addressed your comments and if you have any other changes you'd like to see?

cjdb

Nearly there! Thanks for you patience :)

libcxx/include/__numeric/ranges_iota.h

libcxx/test/std/numerics/numeric.ops/numeric.iota/ranges.iota.pass.cpp

cjdb · 2024-07-29T16:44:05Z

libcxx/test/support/test_iterators.h

+// which we need in certain situations. For example when we want
+// std::weakly_incrementable<Proxy<T>> to be true.
+template <class T>
+struct ProxyDiffTBase {


As a rule of thumb, I don't mind duplication in tests so that things are known to be correct on inspection. Abstraction is fine, but only if it doesn't cast doubt on our confidence that the test is correct.

I think in this case, I'd prefer a proxy_with_difference_type and proxy_missing_difference_type. That might be deserving of its own PR, and before this one is merged so you can use it.

cjdb · 2024-07-29T16:45:00Z

libcxx/test/support/test_iterators.h

+template <class T>
+struct ProxyDiffTBase {
+  // Add default `operator<=>` so that the derived type, Proxy, can also use the default `operator<=>`
+  friend constexpr auto operator<=>(const ProxyDiffTBase&, const ProxyDiffTBase&) = default;


Nit: pass by value (same elsewhere for types that you wouldn't ordinarily pass by ref-to-const).

I agree it seems a bit unnecessary to pass by ref-to-const here, but I was basing these on the existing Proxy<T> which uses ref-to-const

…ripped from algorithm header, adding it back

Zingam · 2025-04-01T08:22:47Z

@jamesETsmith I think it makes sense to create a subissues in #105184 so you can associate this PR with it.

….h for module builds to pass

philnik777

Thanks for the work! LGTM. I don't think you need to wait for @var-const. He's not been doing much work upstream recently, so I'm not sure he'll see this. Maybe wait for a day or two just in case.

jamesETsmith · 2025-04-01T19:28:31Z

@jamesETsmith I think it makes sense to create a subissues in #105184 so you can associate this PR with it.

@Zingam I don't have permission to add this as a subissue, but I agree that seems like a good idea. I don't have much experience contributing to libc++ so I'll defer to more senior people. @ldionne, would it be ok to add this as a subissue?

Zingam · 2025-04-02T06:24:01Z

libcxx/docs/Status/Cxx23Papers.csv

@@ -46,7 +46,7 @@
 "`P2255R2 <https://wg21.link/P2255R2>`__","A type trait to detect reference binding to temporary","2022-02 (Virtual)","|Partial|","","Implemented the type traits only."
 "`P2273R3 <https://wg21.link/P2273R3>`__","Making ``std::unique_ptr`` constexpr","2022-02 (Virtual)","|Complete|","16",""
 "`P2387R3 <https://wg21.link/P2387R3>`__","Pipe support for user-defined range adaptors","2022-02 (Virtual)","|Complete|","19",""
-"`P2440R1 <https://wg21.link/P2440R1>`__","``ranges::iota``, ``ranges::shift_left`` and ``ranges::shift_right``","2022-02 (Virtual)","","",""
+"`P2440R1 <https://wg21.link/P2440R1>`__","``ranges::iota``, ``ranges::shift_left`` and ``ranges::shift_right``","2022-02 (Virtual)","|Partial| ``std::ranges::shift_left`` and ``std::ranges::shift_right`` have not been implemented","","|ranges|"


This line doesn't seem right. The comment should go in the last column I think. Also maybe just claim what's implemented.

Good catch, should be fixed now

@Zingam lmk if it looks good to you now

Zingam · 2025-04-02T10:20:53Z

@jamesETsmith I think it makes sense to create a subissues in #105184 so you can associate this PR with it.

@Zingam I don't have permission to add this as a subissue, but I agree that seems like a good idea. I don't have much experience contributing to libc++ so I'll defer to more senior people. @ldionne, would it be ok to add this as a subissue?

I created the sub-issues for you and associated the PR with it.

… ranges::iota

…ers.csv

… was out of order in the algorithm header which was triggering clang-fmt and clang-tidy CI failures

jamesETsmith · 2025-04-03T14:00:51Z

Just fixed the out of order imports that were causing the clang-fmt and clang-tidy failures on CI.

For reference, I couldn't reproduce the clang-tidy errors locally when running the libcxx/test/libcxx/clang_tidy.gen.py/algorithm.sh.cpp test that triggered one of the CI failures even though I had clearly added out_value_result.h out of order.

Zingam · 2025-04-05T19:35:21Z

@jamesETsmith I think we should have an entry for this in the Release Notes:
https://github.com/llvm/llvm-project/blob/main/libcxx/docs/ReleaseNotes/21.rst

jamesETsmith added 8 commits September 28, 2023 10:11

[libc++] Implement ranges::iota and ranges::out_value_result

c4a3ccf

Merge branch 'main' of https://github.com/llvm/llvm-project into rang…

1b808ec

…es_iota

[libc++] Implement ranges::iota: update module and docs info

574c94c

[libc++] Implement ranges::iota: fixing formatting issues for out_val…

e4cfe00

…ue_result

[libc++] Implement ranges::iota: Cleaning up tests for ranges::iota a…

7cdfea2

…nd out_value_result. Updating Cxx23Papers.csv too.

[libc++] Implement ranges::iota: Cleaning up tests for ranges::iota a…

a1d015c

…nd adding testing for more iterators

[libc++] Implement ranges::iota: Using clang-format-17 to make CI che…

ab4c67d

…cks happy

Merge branch 'main' of https://github.com/llvm/llvm-project into rang…

c44d7d6

…es_iota

jamesETsmith requested a review from a team as a code owner October 7, 2023 18:51

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 7, 2023

philnik777 requested changes Oct 7, 2023

View reviewed changes

philnik777 assigned var-const and philnik777 Oct 7, 2023

[libc++] Implement ranges::iota: Adding helper function to implementa…

a455f42

…tion and updating docs.

jamesETsmith added 2 commits October 26, 2023 16:53

[libc++] Implement ranges::iota: Adding tests to ranges_robust_agains…

08e3c77

…t_* tests (proxy iterators is failing currently).

[libc++] Implement ranges::iota: Merging with main and handling confl…

e658ec2

…icts in docs

[libc++] Implement ranges::iota: Fixing formatting problems

67fcac1

[libc++] Implement ranges::iota: Addressing comments about out_value_…

b132784

…result tests from https://reviews.llvm.org/D121436

philnik777 added the ranges Issues related to `<ranges>` label Oct 27, 2023

[libc++] Implementing ranges::iota: Updating with upstream main and r…

a7e77b1

…esolving merge conflicts

[libc++] Implementing ranges::iota: Accidentally manually updated the…

d50b7a1

… version.version.compile tests rather than generating it with the python script so the formatting was off

strega-nil requested changes Jul 25, 2024

View reviewed changes

[libc++] Implementing ranges::iota: Cleanup and addressing comments f…

b4008ad

…rom @strega-nil

strega-nil approved these changes Jul 27, 2024

View reviewed changes

cjdb requested changes Jul 29, 2024

View reviewed changes

Zingam mentioned this pull request Sep 22, 2024

[libc++][C++23] P2440R1: Implement ranges::iota #109552

Closed

jamesETsmith added 2 commits March 27, 2025 10:16

[libc++] Implementing ranges::iota: Updating with upstream main

240d4f0

[libc++] Implementing ranges::iota: Including out_value_result got st…

8f5ffc3

…ripped from algorithm header, adding it back

[libc++] Implementing ranges::iota: Needed to add __iterator/concepts…

b99c8d8

….h for module builds to pass

philnik777 approved these changes Apr 1, 2025

View reviewed changes

Zingam reviewed Apr 2, 2025

View reviewed changes

jamesETsmith added 3 commits April 2, 2025 12:27

[libc++] implementing ranges::iota: Fixing the Cxx23Paper.csv doc for…

dc29b80

… ranges::iota

[libc++] implementing ranges::iota: Switching the wording in Cxx23Pap…

e325de4

…ers.csv

[libc++] implementing ranges::iota: the header for out_value_result.h…

913a8d8

… was out of order in the algorithm header which was triggering clang-fmt and clang-tidy CI failures

philnik777 merged commit 475cbf0 into llvm:main Apr 5, 2025
85 checks passed

jamesETsmith deleted the ranges_iota branch April 5, 2025 11:49

jamesETsmith mentioned this pull request Apr 5, 2025

[libc++] Updating release notes to include ranges::iota #134511

Closed

		@@ -0,0 +1,124 @@
		//===----------------------------------------------------------------------===//

	constexpr iota_result<_Out, _Tp> operator()(_Out __first, _Sent __last, _Tp __value) const {
	static constexpr iota_result<_Out, _Tp> operator()(_Out __first, _Sent __last, _Tp __value) {

[libc++] Implement ranges::iota #68494

[libc++] Implement ranges::iota #68494

Uh oh!

Conversation

jamesETsmith commented Oct 7, 2023 • edited by Zingam Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

TODOs

Uh oh!

philnik777 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesETsmith Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesETsmith commented Oct 10, 2023

Uh oh!

jamesETsmith commented Oct 24, 2023

Uh oh!

philnik777 commented Oct 25, 2023

Uh oh!

github-actions bot commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesETsmith commented Oct 26, 2023

Uh oh!

philnik777 commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesETsmith commented Oct 27, 2023

Uh oh!

philnik777 commented Oct 27, 2023

Uh oh!

jamesETsmith commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

philnik777 commented Oct 27, 2023

Uh oh!

jamesETsmith commented Oct 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

strega-nil commented Jul 22, 2024

Uh oh!

jamesETsmith commented Jul 22, 2024

Uh oh!

jamesETsmith commented Jul 23, 2024

Uh oh!

jamesETsmith commented Jul 24, 2024

Uh oh!

strega-nil commented Jul 24, 2024

Uh oh!

strega-nil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

jamesETsmith commented Oct 7, 2023 •

edited by Zingam

Loading

jamesETsmith Oct 25, 2023 •

edited

Loading

github-actions bot commented Oct 26, 2023 •

edited

Loading

philnik777 commented Oct 27, 2023 •

edited

Loading

jamesETsmith commented Oct 27, 2023 •

edited

Loading

jamesETsmith commented Oct 27, 2023 •

edited

Loading

Zingam Apr 2, 2025 •

edited

Loading

jamesETsmith commented Apr 3, 2025 •

edited

Loading