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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

JMazurkiewicz
Copy link
Contributor

@JMazurkiewicz JMazurkiewicz commented Sep 6, 2023 •
edited by ldionne
Loading

Fixes #105185

@github-actions github-actions bot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 6, 2023
@JMazurkiewicz JMazurkiewicz force-pushed the libcxx/ranges/join_with branch 5 times, most recently from 3259db4 to 0481670 Compare September 8, 2023 00:07
@ldionne ldionne assigned var-const and huixie90 and unassigned var-const Sep 13, 2023
@ldionne
Copy link
Member

ldionne commented Sep 15, 2023

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

@philnik777 philnik777 added the ranges Issues related to `<ranges>` label Oct 6, 2023
@JMazurkiewicz JMazurkiewicz mentioned this pull request Oct 24, 2023
Merged
ldionne pushed a commit that referenced this pull request Oct 24, 2023
@JMazurkiewicz
[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
@github-actions GitHub Actions
Copy link

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

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

@JMazurkiewicz JMazurkiewicz marked this pull request as ready for review February 4, 2024 14:58
@JMazurkiewicz JMazurkiewicz requested a review from a team as a code owner February 4, 2024 14:58
@H-G-Hristov
Copy link
Contributor

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

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

@JMazurkiewicz JMazurkiewicz force-pushed the libcxx/ranges/join_with branch 3 times, most recently from a84985a to fcdf64a Compare May 14, 2024 14:38
@JMazurkiewicz JMazurkiewicz force-pushed the libcxx/ranges/join_with branch 3 times, most recently from 54a741c to a9a1031 Compare May 22, 2024 23:17
@JMazurkiewicz
[libc++] Implement views::join_with
698bdd6 
* Implement "P2441R2 `views::join_with`" (https://wg21.link/P2441R2)
* Complete implementation of "P2711R1 Making multi-param constructors of views explicit" (https://wg21.link/P2711R1)
* Complete implementation of "P2770R0 Stashing stashing iterators for proper flattening" (https://wg21.link/P2770R0)
@Zingam
Copy link
Contributor

Zingam commented Jun 26, 2024

@JMazurkiewicz We have guidelines for applying [[nodiscard]] since recently (in case you missed the discussion). I think you can/ or should/ apply [[nodiscard]] wherever it is relevant now (operators, etc.).

@ldionne
Copy link
Member

ldionne commented Sep 10, 2024

Gentle ping on this PR -- do you think you'll have time to pick it up or should we find someone else to get it through the finish line?

huixie90
huixie90 requested changes Sep 28, 2024
View reviewed changes
Copy link
Contributor

@huixie90 huixie90 left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. it looks really good. I started looking at the join_with_view.h and has not finished the header yet.

libcxx/include/__ranges/join_with_view.h

namespace ranges {
template <class _Range, class _Pattern>
concept __compatible_joinable_ranges =
Copy link
Contributor

Choose a reason for hiding this comment

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

The spec is specified as concatable after LWG4074. Perhaps directly use the new resolution

libcxx/include/__ranges/join_with_view.h

_LIBCPP_NODISCARD _LIBCPP_HIDE_FROM_ABI constexpr auto begin() const
requires forward_range<const _View> && forward_range<const _Pattern> &&
is_reference_v<range_reference_t<const _View>> && input_range<range_reference_t<const _View>>
Copy link
Contributor

Choose a reason for hiding this comment

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

might as well address LWG4074

libcxx/include/__ranges/join_with_view.h

_LIBCPP_NODISCARD _LIBCPP_HIDE_FROM_ABI constexpr auto end() const
requires forward_range<const _View> && forward_range<const _Pattern> &&
is_reference_v<range_reference_t<const _View>> && input_range<range_reference_t<const _View>>
Copy link
Contributor

Choose a reason for hiding this comment

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

please address LWG4074 since you are adding new code

libcxx/include/__ranges/join_with_view.h
join_with_view(_Range&&, range_value_t<range_reference_t<_Range>>)
-> join_with_view<views::all_t<_Range>, single_view<range_value_t<range_reference_t<_Range>>>>;

template <class _Base, class _PatternBase>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you can add a third template argument class _InnerBase = range_reference_t<_Base> if you want, to simplify the code below a bit, since range_reference_t<_Base> is used a lot of places in the code below and it is already mentioned as InnerBase in the standard

libcxx/include/__ranges/join_with_view.h

static constexpr bool _UseOuterItCache = !forward_range<_View>;
using _OuterItCache = _If<_UseOuterItCache, __non_propagating_cache<iterator_t<_View>>, __empty_cache>;
_LIBCPP_NO_UNIQUE_ADDRESS _OuterItCache __outer_it_;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have a test for this no_unique_address? that is, if we don't need the cache, the test should check that we don't waste the memory

libcxx/include/__ranges/join_with_view.h

static constexpr bool _UseInnerCache = !is_reference_v<_InnerRng>;
using _InnerCache = _If<_UseInnerCache, __non_propagating_cache<remove_cvref_t<_InnerRng>>, __empty_cache>;
_LIBCPP_NO_UNIQUE_ADDRESS _InnerCache __inner_;
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above. do we have a test for the no_unique_address?

huixie90
huixie90 reviewed Sep 29, 2024
View reviewed changes
libcxx/include/__ranges/join_with_view.h

static constexpr bool _OuterIterPresent = forward_range<_Base>;
using _OuterIterType = _If<_OuterIterPresent, _OuterIter, std::__empty>;
_LIBCPP_NO_UNIQUE_ADDRESS _OuterIterType __outer_it_ = _OuterIterType();
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have a test that if we don't store the outer iterator here, we don't waste the space?

libcxx/include/__ranges/join_with_view.h
using _OuterIterType = _If<_OuterIterPresent, _OuterIter, std::__empty>;
_LIBCPP_NO_UNIQUE_ADDRESS _OuterIterType __outer_it_ = _OuterIterType();

variant<_PatternIter, _InnerIter> __inner_it_;
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if the variant is in the valueless_by_exception state? is it worth adding assertions in the member functions we are not in that state?

Copy link
Contributor

Choose a reason for hiding this comment

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

Per current standard wording, some operations are required to throw bad_variant_access. It's unclear whether this is intended, and LWG4059 intends to make such operations UB when valueless_by_exception.

libcxx/include/__ranges/join_with_view.h
}

_LIBCPP_NODISCARD _LIBCPP_HIDE_FROM_ABI constexpr decltype(auto) operator*() const {
using __reference = common_reference_t<iter_reference_t<_InnerIter>, iter_reference_t<_PatternIter>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have tests that the __reference of inner and pattern results in a completely different type?

@JMazurkiewicz
Copy link
Contributor Author

Gentle ping on this PR -- do you think you'll have time to pick it up or should we find someone else to get it through the finish line?

I'll take care of this PR again, starting next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zingam Zingam Zingam left review comments

@frederick-vs-ja frederick-vs-ja frederick-vs-ja left review comments

@huixie90 huixie90 huixie90 requested changes

Assignees

@huixie90 huixie90

Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. ranges Issues related to `<ranges>`
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

P2441R2: views::join_with
8 participants
@JMazurkiewicz @ldionne @H-G-Hristov @Zingam @frederick-vs-ja @huixie90 @var-const @philnik777