Skip to content
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

[libc++][ranges] Implement ranges::stride_view. #65200

Open
wants to merge 138 commits into
base: main
Choose a base branch
from

Conversation

hawkinsw
Copy link
Contributor

@hawkinsw hawkinsw commented Sep 2, 2023

Implement ranges::stride_view in libc++. Migrating to this GitHub from Phabricator (https://reviews.llvm.org/D156924).

Closes #105198

@hawkinsw hawkinsw requested a review from a team as a code owner September 2, 2023 06:55
@cor3ntin cor3ntin added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. ranges Issues related to `<ranges>` labels Sep 2, 2023
@var-const var-const self-assigned this Sep 2, 2023
@hawkinsw
Copy link
Contributor Author

hawkinsw commented Sep 2, 2023

Thank you all for the feedback on this work. I am really excited to get to participate. As I said, I have committed small amounts of code in the past but this is my first attempt at doing a significant amount of implementation. I have already learned a ton and look forward to learning more!

@hawkinsw
Copy link
Contributor Author

hawkinsw commented Sep 4, 2023

I just used a fixup! commit to add some additional code. I followed the new best practices and I hope that I did it correctly. Please let me know if I did not follow the instructions properly!

@hawkinsw
Copy link
Contributor Author

hawkinsw commented Sep 7, 2023

I know how busy you all must be with the transition to GitHub. I am going to continue to work on writing tests and look forward to any feedback you have!

@hawkinsw hawkinsw changed the title WIP: [libc++][ranges] Implement ranges::stride_view. [libc++][ranges] Implement ranges::stride_view. Sep 11, 2023
@hawkinsw
Copy link
Contributor Author

Removed the WIP tag (and added an amend fixup commit to accomplish the same thing with in the git realm). I can't wait to hear how I can improve the implementation!!

Will

using difference_type = range_difference_t<_Base>;
using value_type = range_value_t<_Base>;
using iterator_concept =
_If<random_access_range<_Base>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of this needs the cost of lazy SFINAE. and that's a lot of instantiations. Can we reduce it some?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EricWF Sorry to have to ask ... I know what each of the words "lazy" and "SFINAE" means but in combination I am not entirely sure to what you are referring. Could you point me to something that I use to educate myself so that I can fix this antipattern? Sorry again to have to ask!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EricWF I now realized what you meant and believe that the fixup I am about to submit will resolve this! Thanks again for the feedback!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't be so presumptuous as to Resolve this conversation, but let me know if what I just changed seems better.

forward_iterator_tag,
/* else */ input_iterator_tag >>>;

_LIBCPP_NO_UNIQUE_ADDRESS iterator_t<_Base> __current_ = iterator_t<_Base>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A stateless iterator... interesting....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Although the standard labels this default value as exposition only, I checked that other range adaptor implementations follow that "guidance" (see, e.g., transform_view).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't just about empty types. This also compresses types with tail padding.

libcxx/include/__ranges/stride_view.h Outdated Show resolved Hide resolved
Copy link
Contributor

@JMazurkiewicz JMazurkiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick review of product code. Also, please rebase.

I'm not maintainer, but I've already implemented `views::stride` in microsoft/STL/pull/2981, so my comments might be useful :)

Comment on lines 269 to 285
// [range.stride], stride view
using std::ranges::stride_view;

namespace views {
using std::ranges::views::stride;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std module is available in C++20 as an extensions, we should add language guards:

Suggested change
// [range.stride], stride view
using std::ranges::stride_view;
namespace views {
using std::ranges::views::stride;
}
#if _LIBCPP_STD_VER >= 23
// [range.stride], stride view
using std::ranges::stride_view;
namespace views {
using std::ranges::views::stride;
}
#endif // _LIBCPP_STD_VER >= 23

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just want to make sure that I understand:

In C++20 there is an extension that would enable the use of this module. std::ranges::stride_view is only available as of C++23. So, in the case where this module was used as an extension in a C++20-compatible compiler, without this guard there would be a problem.

I hope that I am understanding correctly! Great catch!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In C++20 there is an extension that would enable the use of this module.

Yes, it is documented here: https://libcxx.llvm.org/Modules.html

std::ranges::stride_view is only available as of C++23. So, in the case where this module was used as an extension in a C++20-compatible compiler, without this guard there would be a problem.

Yes, I've got bitten by it too while implementing views::chunk_by.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I appreciate you pointing this out and I am glad that I understood your point. I believe that I have addressed the comment. If so, would you mind if I marked the conversation resolved?

libcxx/include/module.modulemap.in Outdated Show resolved Hide resolved
libcxx/include/__ranges/stride_view.h Outdated Show resolved Hide resolved
libcxx/include/__ranges/stride_view.h Outdated Show resolved Hide resolved
libcxx/include/__ranges/stride_view.h Outdated Show resolved Hide resolved
libcxx/include/__ranges/stride_view.h Outdated Show resolved Hide resolved
libcxx/include/__ranges/stride_view.h Outdated Show resolved Hide resolved
libcxx/include/__ranges/stride_view.h Outdated Show resolved Hide resolved
@hawkinsw
Copy link
Contributor Author

@JMazurkiewicz Thank you for your incredibly helpful comments. I have a few things to do at $dayjob but I will address them as soon as possible. Thank you again for taking the time to comment.

@hawkinsw
Copy link
Contributor Author

Hello again and sorry to bother. I would love to make any changes to the implementation that you think are necessary. I have really enjoyed the opportunity to implement stride_view and I have already started on slide_view!

@hawkinsw hawkinsw self-assigned this Sep 27, 2023
@Zingam
Copy link
Contributor

Zingam commented Sep 29, 2023

Hello again and sorry to bother. I would love to make any changes to the implementation that you think are necessary. I have really enjoyed the opportunity to implement stride_view and I have already started on slide_view!

Please update the status page and any papers: libcxx/docs/Status/RangesViews.csv
Also this PR needs a lot more tests.

@hawkinsw
Copy link
Contributor Author

Hello again and sorry to bother. I would love to make any changes to the implementation that you think are necessary. I have really enjoyed the opportunity to implement stride_view and I have already started on slide_view!

Please update the status page and any papers: libcxx/docs/Status/RangesViews.csv Also this PR needs a lot more tests.

Thank you for the feedback! Are you asking for an update to remove you as an implementer? Refer to the this URL for the PR? I believe that the paper listed in the CSV is the right one? I am sorry if I am missing something obvious.

@Zingam
Copy link
Contributor

Zingam commented Sep 30, 2023

Thank you for the feedback! Are you asking for an update to remove you as an implementer? Refer to the this URL for the PR? I believe that the paper listed in the CSV is the right one? I am sorry if I am missing something obvious.

Please change the URL to point to this PR, also mark which papers this PR implements as complete. You probably need to add the paper to the release notes, when this PR is approved.

Copy link
Member

@var-const var-const left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the long delay! I only went through the stride_view.h header and haven't looked at the tests closely yet, but I wanted to give you the feedback I have so far. Thanks a lot for all the work on the patch!

libcxx/include/__ranges/stride_view.h Show resolved Hide resolved
libcxx/include/__ranges/stride_view.h Show resolved Hide resolved
libcxx/include/__ranges/stride_view.h Show resolved Hide resolved

public:
_LIBCPP_HIDE_FROM_ABI constexpr explicit stride_view(_View __base, range_difference_t<_View> __stride)
: __base_(std::move(__base)), __stride_(__stride) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we test with a move-only __base?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that I addressed this in a93fef8

libcxx/include/__ranges/stride_view.h Outdated Show resolved Hide resolved
libcxx/include/__ranges/stride_view.h Show resolved Hide resolved

_LIBCPP_HIDE_FROM_ABI friend constexpr range_rvalue_reference_t<_Base>
iter_move(__iterator const& __it) noexcept(noexcept(ranges::iter_move(__it.__current_))) {
return ranges::iter_move(__it.__current_);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: do we have tests for the customization point (here and for iter_swap)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this is addressed in 8405d877bc86bde51c4ec61ca7811065c69f1149.

libcxx/include/__ranges/stride_view.h Outdated Show resolved Hide resolved
struct __fn {
template <viewable_range _Range>
[[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Range&& __range, range_difference_t<_Range> __n) const
noexcept(noexcept(stride_view{std::forward<_Range>(__range), __n}))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be slightly easier to read if the three repetitions of the expression were vertically aligned. I think the best way to achieve that is using a /* */ comment like this -- see e.g. as_rvalue_view.h for an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever use of the block comment! I believe that this is addressed in accfb24af355238748e22653b46ed3c08db2208d.

libcxx/include/__ranges/stride_view.h Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Oct 6, 2023

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 6e0fc155782ff5307245a85c7b037a2998ec6c86 135dcbc4a59d43a9608523db2eb204c8b4333f83 --extensions ,cpp,inc,h -- libcxx/include/__ranges/stride_view.h libcxx/test/libcxx/ranges/range.adaptors/range.stride.view/base.nodiscard.verify.cpp libcxx/test/libcxx/ranges/range.adaptors/range.stride.view/begin.nodiscard.verify.cpp libcxx/test/libcxx/ranges/range.adaptors/range.stride.view/ctor.assert.pass.cpp libcxx/test/libcxx/ranges/range.adaptors/range.stride.view/ctor.nodiscard.verify.cpp libcxx/test/libcxx/ranges/range.adaptors/range.stride.view/end.nodiscard.verify.cpp libcxx/test/libcxx/ranges/range.adaptors/range.stride.view/iterator/base.nodiscard.verify.cpp libcxx/test/libcxx/ranges/range.adaptors/range.stride.view/iterator/dereference.nodiscard.verify.cpp libcxx/test/libcxx/ranges/range.adaptors/range.stride.view/iterator/dereference.pass.cpp libcxx/test/libcxx/ranges/range.adaptors/range.stride.view/iterator/increment.pass.cpp libcxx/test/libcxx/ranges/range.adaptors/range.stride.view/iterator/iter_move.nodiscard.verify.cpp libcxx/test/libcxx/ranges/range.adaptors/range.stride.view/iterator/operator.nodiscard.verify.cpp libcxx/test/libcxx/ranges/range.adaptors/range.stride.view/iterator/operator_plus_equal.pass.cpp libcxx/test/libcxx/ranges/range.adaptors/range.stride.view/iterator/subscript.nodiscard.verify.cpp libcxx/test/libcxx/ranges/range.adaptors/range.stride.view/size.nodiscard.verify.cpp libcxx/test/libcxx/ranges/range.adaptors/range.stride.view/stride.nodiscard.verify.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/adaptor.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/base.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/begin.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/borrowing.compile.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/concept.verify.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/ctad.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/ctor.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/end.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/base.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/ctor.copy.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/ctor.default.verify.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/decrement.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/equal.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/greater_than.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/increment.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/iter_move.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/iter_swap.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/less_than.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/operator.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/plus_equal.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/size.verify.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/stride.pass.cpp libcxx/test/std/ranges/range.adaptors/range.stride.view/types.h libcxx/include/ranges libcxx/include/version libcxx/modules/std/ranges.inc libcxx/test/std/language.support/support.limits/support.limits.general/ranges.version.compile.pass.cpp libcxx/test/std/language.support/support.limits/support.limits.general/version.version.compile.pass.cpp libcxx/test/std/ranges/range.adaptors/range.as.rvalue/enable_borrowed_range.compile.pass.cpp
View the diff from clang-format here.
diff --git a/libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/base.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/base.pass.cpp
index df30e0a10f..32a41eec9b 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/base.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/base.pass.cpp
@@ -89,8 +89,8 @@ bool base_copy() {
   [[maybe_unused]] auto svi = sv.begin();
 
   // See above.
-  move_counter                                      = 0;
-  copy_counter                                      = 0;
+  move_counter                                  = 0;
+  copy_counter                                  = 0;
   [[maybe_unused]] const SizedInputIter& result = svi.base();
 
   // Ensure that base did _not_ std::move'd the iterator.
diff --git a/libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/increment.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/increment.pass.cpp
index 5b41bde521..036605d065 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/increment.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/increment.pass.cpp
@@ -196,8 +196,7 @@ int main(int, char**) {
 
   {
     int arr[]{1, 2, 3, 4, 5, 6, 7, 8, 9, 10};
-    test_non_forward_operator_increment(
-        SizedInputIter(arr), SizedInputIter(arr + 3), SizedInputIter(arr + 10));
+    test_non_forward_operator_increment(SizedInputIter(arr), SizedInputIter(arr + 3), SizedInputIter(arr + 10));
   }
 
   test_properly_handling_missing();
diff --git a/libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/plus_equal.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/plus_equal.pass.cpp
index 6d8ae7dec2..41f2c217b4 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/plus_equal.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.stride.view/iterator/plus_equal.pass.cpp
@@ -75,9 +75,9 @@ constexpr bool test_random_access_operator_plus_equal(Iter begin, Iter end, Diff
   assert(*sv_bv_begin == *sv_bv_offset_begin);
   assert(*sv_bv_begin_after_distance == *sv_bv_offset_begin);
 
-  auto big_step              = (end - 1) - begin;
+  auto big_step                            = (end - 1) - begin;
   auto stride_view_over_base_view_big_step = std::ranges::stride_view(base_view, big_step);
-  sv_bv_begin                = stride_view_over_base_view_big_step.begin();
+  sv_bv_begin                              = stride_view_over_base_view_big_step.begin();
 
   // This += should move us into a position where the __missing_ will come into play.
   // Do a -= 1 here to confirm that the __missing_ is taken into account.

Relocate stride view in ranges.inc; update and test enable borrowed range.
Fix misnamed template parameter.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
Add macro push/pop to stride_view.h.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
Make sure not to use reserved names for templates in stride_view.h.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
Explicitly include vector.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
Fix unused variables and (hopefully) missing __assert include.
Updating tests based on feedback.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
Add check for whether iterator::base is move/copy

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
clang-format

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
Add additional testing for sentinel mathematics.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
Make sure that all nodiscard-marked member functions are checked.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
Fix bad rebase.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
Finish fixing bad rebase.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
Format.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
Format.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
Format.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
Begin splitting iterator tests into different files.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
Clean up the iterator increment tests.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
Fix minor format error.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
Refactor and add additional tests.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
@ldionne
Copy link
Member

ldionne commented Sep 10, 2024

@hawkinsw Is this PR ready for another round of review? I'd really like to finish this one since it has been around for a while.

@hawkinsw
Copy link
Contributor Author

@hawkinsw Is this PR ready for another round of review? I'd really like to finish this one since it has been around for a while.

We are getting there. I did some pretty good refactoring of the test cases last night. I should have more time to work on it tonight. I am going as fast as I can. Sorry!
Will

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. ranges Issues related to `<ranges>`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P1899R3: stride_view