This is an archive of the discontinued LLVM Phabricator instance.

WIP: [libc++][ranges] Implement `ranges::stride_view`.
AbandonedPublic

Authored by hawkinsw on Aug 2 2023, 11:08 AM.

Details

Reviewers
ldionne
philnik
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

hawkinsw created this revision.Aug 2 2023, 11:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2023, 11:08 AM
hawkinsw requested review of this revision.Aug 2 2023, 11:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 2 2023, 11:08 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Hello @ldionne and @philnik !! I am a *huge* fan of both of yours and really appreciate all the work that you devote to libc++. I saw a chance to contribute and wanted to try to help.

The code I have here is very rough and early, but I was hoping that you could provide some rough guidance! I have contributed small patches before but this is a new area for me and I have already learned a ton! I know that I need to complete the steps listed in the Pre-commit check list in docs/Contributing.rst and I am working on that now.

Thank you, again, for all the great work!

PS: I hope that you are the right ones to tag for review. I based the tag here on git blame output and I apologize if I am spamming you!

I hope that this contribution is the first of many (e.g., ranges::adjacent_view).
Will

Thanks for working on this!

I'm not to familiar with ranges to I leave a real review to others. I mainly glossed over the patch.

Based on https://libcxx.llvm.org/Status/Ranges @H-G-Hristov is working on this too, @H-G-Hristov what is the status of your work?

If you want to work on a different view nobody is working on please let us know, we can put your name on it.

libcxx/include/__ranges/stride_view.h
188

This line is too long, please use clang-format to format the patch.

Thanks for working on this!

I'm not to familiar with ranges to I leave a real review to others. I mainly glossed over the patch.

Based on https://libcxx.llvm.org/Status/Ranges @H-G-Hristov is working on this too, @H-G-Hristov what is the status of your work?

If you want to work on a different view nobody is working on please let us know, we can put your name on it.

I am really eager to help however I can! I am a huge language geek and love open source. The past two days of coding on this little side project have been the most eye-opening C++ experience I have had since I started (mid 90s). If @H-G-Hristov wants to take over, that's great -- just assign me whatever needs to be done!

Will

libcxx/include/__ranges/stride_view.h
188

Will do!!

This is actually a meta question -- I ran clang-format (in the libcxx/ directory with the .clang-format file) and noticed that the result was nothing like what the other files looked like. I was worried that I had done something wrong.

Thanks for working on this!

I'm not to familiar with ranges to I leave a real review to others. I mainly glossed over the patch.

Based on https://libcxx.llvm.org/Status/Ranges @H-G-Hristov is working on this too, @H-G-Hristov what is the status of your work?

If you want to work on a different view nobody is working on please let us know, we can put your name on it.

I am really eager to help however I can! I am a huge language geek and love open source. The past two days of coding on this little side project have been the most eye-opening C++ experience I have had since I started (mid 90s). If @H-G-Hristov wants to take over, that's great -- just assign me whatever needs to be done!

Will

I think nobody is working on the C++23 range based algorithms. I found some patches for other views. Unfortunately the Phabricator search is under heavy load so I can't search. Maybe visit our Discord channel and reach out to @varconst or @philnik they are most familiar in this area. If you don't know where our Discord is, the link is at the top of https://libcxx.llvm.org/Contributing.html

libcxx/include/__ranges/stride_view.h
188

Not all files are formatted. Currently we use clang-format-17. We like new files to be formatted.

Pressed submit too fast ;-)

I am really eager to help however I can! I am a huge language geek and love open source. The past two days of coding on this little side project have been the most eye-opening C++ experience I have had since I started (mid 90s).

Cool to hear! I too love to see the progress C++ has made over the years :-)

hawkinsw updated this revision to Diff 546669.Aug 2 2023, 7:14 PM

Formatted patch using clang-format on @Mordante 's helpful suggestion.

hawkinsw marked 2 inline comments as done.Aug 2 2023, 10:58 PM

Thank you, @Mordante !

libcxx/include/__ranges/stride_view.h
53

Yes, I realize that this is crazy! It is dead code and I will eliminate it.

H-G-Hristov added a comment.EditedAug 3 2023, 12:48 PM

@Mordante @hawkinsw My patch is about the same state like this one. I am still working on enumerate_view I need to add more tests before I submit it. I didn't have much time to work during the last month but I'd like to get it in Clang 18. Maybe its about time to submit a draft.

@hawkinsw You can continue working on it if you like or maybe you can pick some of the other view which are more useful and interesting but I think also more difficult (join_with and join).
You can update the status here: https://libcxx.llvm.org/Status/Ranges.html

@Mordante @hawkinsw My patch is about the same state like this one. I am still working on enumerate_view I need to add more tests before I submit it. I didn't have much time to work during the last month but I'd like to get it in Clang 18. Maybe its about time to submit a draft.

@hawkinsw You can continue working on it if you like or maybe you can pick some of the other view which are more useful and interesting but I think also more difficult (join_with and join).
You can update the status here: https://libcxx.llvm.org/Status/Ranges.html

Hello @H-G-Hristov !! As a warmup to working on others, I will continue to hammer on this (my next goal is to write tests!) I don't want to waste your time (obviously!) but it's been fun to work on this and a great learning experience. The next goal I have is to write tests (I will start on that tonight!)

Does that sound okay?

@Mordante @hawkinsw My patch is about the same state like this one. I am still working on enumerate_view I need to add more tests before I submit it. I didn't have much time to work during the last month but I'd like to get it in Clang 18. Maybe its about time to submit a draft.

@hawkinsw You can continue working on it if you like or maybe you can pick some of the other view which are more useful and interesting but I think also more difficult (join_with and join).
You can update the status here: https://libcxx.llvm.org/Status/Ranges.html

Hello @H-G-Hristov !! As a warmup to working on others, I will continue to hammer on this (my next goal is to write tests!) I don't want to waste your time (obviously!) but it's been fun to work on this and a great learning experience. The next goal I have is to write tests (I will start on that tonight!)

Does that sound okay?

Sure, please continue.

hawkinsw updated this revision to Diff 547083.Aug 3 2023, 7:28 PM

Marked div_ceil as ABI invisible and removed caching.

hawkinsw updated this revision to Diff 547084.Aug 3 2023, 7:35 PM

Fixed the commit against which this diff should be applied.

hawkinsw updated this revision to Diff 547209.Aug 4 2023, 7:40 AM

Remove inclusion of spurious change to libcxx-lit.

hawkinsw updated this revision to Diff 547242.Aug 4 2023, 9:06 AM

clang-tidy'd the code.

hawkinsw updated this revision to Diff 547386.Aug 4 2023, 3:57 PM

Added a first, basic test and also did some more clang-tidy'ing.

@hawkinsw You can look at my WIP for stuff you can easily copy: https://reviews.llvm.org/D157192

@hawkinsw You can look at my WIP for stuff you can easily copy: https://reviews.llvm.org/D157192

Thank you -- very kind of you! Two heads (especially yours -- which is smarter than mine!) are better than one!

hawkinsw updated this revision to Diff 547953.Aug 7 2023, 2:40 PM

Fix some ADL warnings, compilation errors and implementation details.

hawkinsw changed the edit policy from "All Users" to "Custom Policy".Aug 7 2023, 2:52 PM
hawkinsw updated this revision to Diff 547960.Aug 7 2023, 2:54 PM

Bring in the awesome tests that @H-G-Hristov already wrote!

hawkinsw updated this revision to Diff 547983.Aug 7 2023, 3:41 PM

More cargo crating from @H-G-Hristov's excellent work.

hawkinsw updated this revision to Diff 548041.Aug 7 2023, 8:07 PM

Check in the modified feature test macro code.

hawkinsw updated this revision to Diff 548065.Aug 7 2023, 10:47 PM

Adding some missing include files.

hawkinsw updated this revision to Diff 548382.Aug 8 2023, 3:51 PM

Make div-ceil system only by adding __ prefix.

hawkinsw updated this revision to Diff 548446.Aug 8 2023, 8:07 PM

Add initial tests for ::size

hawkinsw updated this revision to Diff 548607.Aug 9 2023, 7:08 AM

(maybe) last clang-tidy/format and add base tests.

hawkinsw updated this revision to Diff 548610.Aug 9 2023, 7:15 AM

Clean up the skeleton test file (remove references to std::ranges::enumerate).

@H-G-Hristov Thank you for all the contributions -- your code was immensely helpful. I think that I am making some progress! If you have any thoughts on the way that things are going with the implementation, please let me know! Thanks again!

H-G-Hristov added a comment.EditedAug 9 2023, 2:52 PM

@H-G-Hristov Thank you for all the contributions -- your code was immensely helpful. I think that I am making some progress! If you have any thoughts on the way that things are going with the implementation, please let me know! Thanks again!

I'm also a rookie at this, so treat anything with a pinch of salt. This is may WIP patch for enumerate_view: https://reviews.llvm.org/D157193 I think it has some similarities to stride_view. It's missing a few converting constructor tests but after rebase it should get green. I do expect that it will get a lot of scrutiny and feedback in the review but I think you can check it for what tests you need to write. I've based my tests on already existing tests.

I am also going to update the status page with my patch soon: https://libcxx.llvm.org/Status/Ranges.html If you like I could update it for you too. Or you can do it yourself.

N.B. You should update the synopses. The implemented papers/issues, etc.

hawkinsw updated this revision to Diff 548882.Aug 9 2023, 11:10 PM

Prevent __to_unsigned_like from using ADL.

hawkinsw updated this revision to Diff 549476.Aug 11 2023, 11:49 AM

Add iterator operator== tests.

hawkinsw updated this revision to Diff 552012.Aug 21 2023, 7:22 AM

Adding some tests (generally making progress).

hawkinsw updated this revision to Diff 555503.Sep 1 2023, 1:59 PM

Add tests for enable_borrowed_range.

@hawkinsw First of all, thanks a ton for working on this! LLVM is currently in the process of moving away from Phabricator to GitHub reviews -- see https://discourse.llvm.org/t/logistics-of-the-transition-to-github-prs-for-libc-libc-abi-libunwind/73130. Since the review for this patch hasn't really started yet, I think it might be easier to open a new review on GitHub. Would that work for you? If you have a strong preference to keep the patch on Phabricator for now, that's ok, I can start the review here.

Btw, can you give a very brief outline on the current status of the patch? I notice it's still marked as WIP so I'm just curious which parts are still TODO. Would it make sense to start reviewing it now, or should I wait a bit? Let me know, and once again, thanks a lot for working on this!

@hawkinsw First of all, thanks a ton for working on this! LLVM is currently in the process of moving away from Phabricator to GitHub reviews -- see https://discourse.llvm.org/t/logistics-of-the-transition-to-github-prs-for-libc-libc-abi-libunwind/73130. Since the review for this patch hasn't really started yet, I think it might be easier to open a new review on GitHub. Would that work for you? If you have a strong preference to keep the patch on Phabricator for now, that's ok, I can start the review here.

You got it!! I want to make sure that I do what the community wants! I will move it there and then comment back here and let you know where it lands!

Btw, can you give a very brief outline on the current status of the patch? I notice it's still marked as WIP so I'm just curious which parts are still TODO. Would it make sense to start reviewing it now, or should I wait a bit? Let me know, and once again, thanks a lot for working on this!

Though I am a long-time C++ developer, I have only submitted minor optimizations to STL implementations to date. So, I _think_ that what is here is something that is roughly complete. What I am adding now are tests and it's taking a little longer than I had hoped. My day job as a teacher just ramped up again.

That is to say, I would love to start getting feedback on the patch and start refining it! I have had so much fun doing the implementation so far and learned a ton. I know that your review will help me learn even more!

Thank you for being so encouraging.

hawkinsw abandoned this revision.Sep 1 2023, 11:56 PM

In light of the transition to GitHub PRs, I am closing this revision in favor of https://github.com/llvm/llvm-project/pull/65200.