[libc++] Implement `views::join_with` #65536

JMazurkiewicz · 2023-09-06T21:40:41Z

Implement "P2441R2 views::join_with" (https://wg21.link/P2441R2), closes P2441R2: views::join_with #105185
Implement LWG4074 (https://wg21.link/LWG4074), closes LWG4074: compatible-joinable-ranges is underconstrained #105346
Complete implementation of "P2711R1 Making multi-param constructors of views explicit" (https://wg21.link/P2711R1), closes P2711R1: Making multi-param constructors of views explicit #105252
Complete implementation of "P2770R0 Stashing stashing iterators for proper flattening" (https://wg21.link/P2770R0), closes P2770R0: Stashing stashing iterators for proper flattening #105250

libcxx/docs/Status/Cxx23Papers.csv

ldionne · 2023-09-15T20:55:42Z

@JMazurkiewicz Please ping us when this is ready to review and make it a non-draft.

This patch adds a mention that the following papers are in progress: * P2770R0: #66033 * P2441R2 and P2711R1: #65536

github-actions · 2024-02-03T00:00:05Z

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

H-G-Hristov · 2024-05-10T14:27:22Z

I guess with constexpr variant this cool feature can be finalized now :)

…with

huixie90 · 2024-12-25T15:12:52Z

libcxx/include/__ranges/join_with_view.h

+  _LIBCPP_HIDE_FROM_ABI constexpr __iterator(__iterator<!_Const> __i)
+    requires _Const && convertible_to<iterator_t<_View>, _OuterIter> &&
+                 convertible_to<iterator_t<_InnerRng>, _InnerIter> && convertible_to<iterator_t<_Pattern>, _PatternIter>
+      : __parent_(__i.__parent_), __outer_it_(std::move(__i.__outer_it_)) {


I think there is something wrong. __iterator<false> is not a friend of __iterator<true>, so in theory this should not compile because of accessing private members.

This probably indicates that this function is not tested

I think there is something wrong. __iterator<false> is not a friend of __iterator<true>, so in theory this should not compile because of accessing private members.

This probably indicates that this function is not tested

I think that everything is OK here. Even though __iterator<false> is not a friend of __iterator<true>, its members can still be accessed thanks to friend join_with_view declaration. Per [class.friend]/2:

Declaring a class to be a friend implies that private and protected members of the class granting friendship can be named in the base-specifiers and member declarations of the befriended class.

__iterator<false> declares join_with_view to be a friend, which means that member declarations of it (__iterator<true> and __sentinel<?>) can access iterator's private members.

huixie90 · 2024-12-25T15:15:45Z

...t/std/ranges/range.adaptors/range.join.with/range.join.with.iterator/ctor.not_const.pass.cpp

+    auto it = jwv.begin();
+    assert(*it == 1);
+
+    using CIter      = std::ranges::iterator_t<const decltype(jwv)>;


Could you please

static_assert(!std::is_same_v<Iter, CIter>);

I think this test is not testing what it is supposed to test. I suspect it and cit are both iterator<true>

Added here: 14471bc

huixie90 · 2024-12-25T15:29:49Z

libcxx/include/__ranges/join_with_view.h

+
+  _LIBCPP_HIDE_FROM_ABI constexpr __sentinel(__sentinel<!_Const> __s)
+    requires _Const && convertible_to<sentinel_t<_View>, sentinel_t<_Base>>
+      : __end_(std::move(__s.__end_)) {}


same as iterator, I think __sentinel<false> is not a friend of __sentinel<true> so this should not compile. And it is an indication of missing tests.

I think that [class.friend]/2 applies here too.

huixie90 · 2024-12-25T15:32:06Z

libcxx/include/__ranges/join_with_view.h

+    requires sentinel_for<sentinel_t<_Base>, iterator_t<__maybe_const<_OtherConst, _View>>>
+  [[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI friend constexpr bool
+  operator==(const __iterator<_OtherConst>& __x, const __sentinel& __y) {
+    return __x.__get_outer() == __y.__end_;


again. __get_outer is private and I don't think sentinel has been befriended to iterator. This should not compile in theory. possible missing tests as well

You are right here, operator== is a friend of sentinel only, so it should not be able to access iterator's private members. Unluckily, both clang and gcc accept this incorrect code.

Fixed here: 9760ec3

huixie90 · 2024-12-25T16:54:28Z

...est/std/ranges/range.adaptors/range.join.with/range.join.with.iterator/ctor.default.pass.cpp

+
+// <ranges>
+
+// iterator() = default;


we usually have another test point to test this is not explicit.

Iter iter = {};

Added here: 3771be9

huixie90 · 2024-12-25T17:18:33Z

...x/test/std/ranges/range.adaptors/range.join.with/range.join.with.iterator/iter_swap.pass.cpp

+
+// <ranges>
+
+// friend constexpr void iter_swap(const iterator& x, const iterator& y);


can we have a test to test that it delicates to underlying's tier_swap? i.e. not doing ranges::swap(*it1, *it2)

Done! 19e4fa2

huixie90 · 2024-12-25T17:20:02Z

...t/std/ranges/range.adaptors/range.join.with/range.join.with.sentinel/ctor.non_const.pass.cpp

+    std::ranges::join_with_view jwv(vec, 0);
+    using JWV = decltype(jwv);
+    static_assert(!std::ranges::common_range<JWV>);
+    using CSent = std::ranges::sentinel_t<const JWV>;


can we static_assert(!std::same_as<Sent, CSent>) to make sure we are testing what we are supposed to test

Added here: d52bdbf

huixie90 · 2024-12-25T17:21:07Z

libcxx/test/std/ranges/range.adaptors/range.join.with/range.join.with.sentinel/eq.pass.cpp

+
+// template<bool OtherConst>
+//   requires sentinel_for<sentinel_t<Base>, iterator_t<maybe-const<OtherConst, V>>>
+// friend constexpr bool operator==(const iterator<OtherConst>& x, const sentinel& y);


can we make sure we static_assert(!same_as<Iter, CIter>) and static_assert(!same_as<Sent, CSent>) and make sure we test all the combinations

Added here: 96eae0b

huixie90 · 2024-12-26T15:11:34Z

...xx/test/std/ranges/range.adaptors/range.join.with/range.join.with.view/ctor.default.pass.cpp

+
+// <ranges>
+
+// join_with_view()


we usually have a test to test the explicitness

join_with_view<...> jwv = {};

Added: 4ab1ee2

huixie90 · 2024-12-26T15:18:26Z

libcxx/test/std/ranges/range.adaptors/range.join.with/range.join.with.view/end.pass.cpp

+  }
+}
+
+constexpr void test_end() {}


why is this needed?

It's not, I forgot to remove it earlier (fb5ab01).

Comment: llvm#65536 (comment)

…structor Comment: llvm#65536 (comment)

Comment: llvm#65536 (comment)

…`false` Comment: llvm#65536 (comment)

…ove`) Comment: llvm#65536 (comment)

…wap`) Comment: llvm#65536 (comment)

…s.cpp`) Comment: llvm#65536 (comment)

Comment: llvm#65536 (comment)

…s.cpp`) Comment: llvm#65536 (comment)

Comment: llvm#65536 (comment) Drive-by: make use of new aliases

Comment: llvm#65536 (comment)

…with

Zingam · 2025-01-18T08:58:27Z

@JMazurkiewicz At the top you mention that this PR completed several papers. Do you need to add additional GitHub issues to the list that this PR closes/fixes/?

…with

Instead of `// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20`

github-actions bot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 6, 2023

JMazurkiewicz force-pushed the libcxx/ranges/join_with branch 5 times, most recently from 3259db4 to 0481670 Compare September 8, 2023 00:07

Zingam reviewed Sep 8, 2023

View reviewed changes

libcxx/docs/Status/Cxx23Papers.csv Outdated Show resolved Hide resolved

JMazurkiewicz force-pushed the libcxx/ranges/join_with branch from 0481670 to f113229 Compare September 8, 2023 15:11

ldionne assigned var-const and huixie90 and unassigned var-const Sep 13, 2023

ldionne requested a review from huixie90 September 15, 2023 20:55

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

JMazurkiewicz mentioned this pull request Oct 24, 2023

[libc++] Update status of P2770R0, P2441R2 and P2711R1 papers #70045

Merged

ldionne pushed a commit that referenced this pull request Oct 24, 2023

[libc++] Update status of P2770R0, P2441R2 and P2711R1 (#70045)

e1e0598

This patch adds a mention that the following papers are in progress: * P2770R0: #66033 * P2441R2 and P2711R1: #65536

JMazurkiewicz force-pushed the libcxx/ranges/join_with branch from f113229 to 1835f68 Compare February 2, 2024 23:57

JMazurkiewicz marked this pull request as ready for review February 4, 2024 14:58

JMazurkiewicz requested a review from a team as a code owner February 4, 2024 14:58

JMazurkiewicz force-pushed the libcxx/ranges/join_with branch 3 times, most recently from a84985a to fcdf64a Compare May 14, 2024 14:38

JMazurkiewicz force-pushed the libcxx/ranges/join_with branch 3 times, most recently from 54a741c to a9a1031 Compare May 22, 2024 23:17

JMazurkiewicz force-pushed the libcxx/ranges/join_with branch from a9a1031 to 9406093 Compare June 5, 2024 09:49

JMazurkiewicz added 7 commits November 29, 2024 00:09

Various cleanups

5a3996a

Merge remote-tracking branch 'upstream/main' into libcxx/ranges/join_…

8b3e36c

…with

Fix formatting

ee942ce

Fix example.pass.cpp test

56d8850

Fix 19.rst file

15efa6f

Enable iterator/no_unique_address.compile.pass.cpp test for MSVC

f267d76

Fix formatting (again)

3ad462c

huixie90 requested changes Dec 26, 2024

View reviewed changes

JMazurkiewicz added 17 commits January 9, 2025 16:00

Remove empty test_end function

fb5ab01

Comment: llvm#65536 (comment)

Test explicitness of default constructor of join_with_view

4ab1ee2

Comment: llvm#65536 (comment)

Test explicitness of join_with_view::iterator(iterator<!Const>) con…

bf1eeac

…structor Comment: llvm#65536 (comment)

Test explicitness of default constructor of join_with_view::iterator

3771be9

Comment: llvm#65536 (comment)

Parametrise test_pre_increment on ref-is-glvalue being true or …

04942c0

…`false` Comment: llvm#65536 (comment)

Add extra test for iter_move (make sure it calls underlying `iter_m…

42ce9c2

…ove`) Comment: llvm#65536 (comment)

Remove GCC workaround from increment.pass.cpp

21896e9

Add extra test for iter_swap (make sure it calls underlying `iter_s…

19e4fa2

…wap`) Comment: llvm#65536 (comment)

Make sure CIter and Iter are different types (`ctor.not_const.pas…

14471bc

…s.cpp`) Comment: llvm#65536 (comment)

Make sure CIter and Iter are different types (eq.pass.cpp)

52ad025

Comment: llvm#65536 (comment)

Make sure CSent and Sent are different types (`ctor.not_const.pas…

d52bdbf

…s.cpp`) Comment: llvm#65536 (comment)

Make sure CSent and Sent are different types (eq.pass.cpp)

96eae0b

Comment: llvm#65536 (comment) Drive-by: make use of new aliases

Add missing <array> include in iter_swap.pass.cpp

0dc76bb

Fix sentinel::operator==

9760ec3

Comment: llvm#65536 (comment)

Merge remote-tracking branch 'upstream/main' into libcxx/ranges/join_…

8ebaa0c

…with

Fix formatting

d9d0033

Clarify some comments in test_post_increment

d3c014f

JMazurkiewicz added 4 commits January 28, 2025 22:12

Use _LIBCPP_NODEBUG attribute

abe8ab9

Merge remote-tracking branch 'upstream/main' into libcxx/ranges/join_…

4972ccc

…with

[NFC] Use // REQUIRES: std-at-least-c++23

4601812

Instead of `// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20`

More _LIBCPP_NODEBUG

9847c22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libc++] Implement `views::join_with` #65536

[libc++] Implement `views::join_with` #65536

JMazurkiewicz commented Sep 6, 2023 •

edited

Loading

ldionne commented Sep 15, 2023

github-actions bot commented Feb 3, 2024 •

edited

Loading

H-G-Hristov commented May 10, 2024 •

edited

Loading

huixie90 Dec 25, 2024

JMazurkiewicz Jan 16, 2025 •

edited

Loading

huixie90 Dec 25, 2024

JMazurkiewicz Jan 16, 2025

huixie90 Dec 25, 2024

JMazurkiewicz Jan 16, 2025

huixie90 Dec 25, 2024

JMazurkiewicz Jan 16, 2025

huixie90 Dec 25, 2024

JMazurkiewicz Jan 16, 2025

huixie90 Dec 25, 2024

JMazurkiewicz Jan 16, 2025

huixie90 Dec 25, 2024

JMazurkiewicz Jan 16, 2025

huixie90 Dec 25, 2024

JMazurkiewicz Jan 16, 2025

huixie90 Dec 26, 2024

JMazurkiewicz Jan 16, 2025

huixie90 Dec 26, 2024

JMazurkiewicz Jan 16, 2025

Zingam commented Jan 18, 2025


		// <ranges>

		// friend constexpr void iter_swap(const iterator& x, const iterator& y);

[libc++] Implement views::join_with #65536

Are you sure you want to change the base?

[libc++] Implement views::join_with #65536

Conversation

JMazurkiewicz commented Sep 6, 2023 • edited Loading

ldionne commented Sep 15, 2023

github-actions bot commented Feb 3, 2024 • edited Loading

H-G-Hristov commented May 10, 2024 • edited Loading

Choose a reason for hiding this comment

JMazurkiewicz Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zingam commented Jan 18, 2025

[libc++] Implement `views::join_with` #65536

[libc++] Implement `views::join_with` #65536

JMazurkiewicz commented Sep 6, 2023 •

edited

Loading

github-actions bot commented Feb 3, 2024 •

edited

Loading

H-G-Hristov commented May 10, 2024 •

edited

Loading

JMazurkiewicz Jan 16, 2025 •

edited

Loading