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

[range.adaptors, ranges.syn] Various editorial fixes #2596

Merged
merged 2 commits into from Dec 19, 2018
Merged

[range.adaptors, ranges.syn] Various editorial fixes #2596

merged 2 commits into from Dec 19, 2018

Conversation

JohelEGP
Copy link
Contributor

No description provided.

jensmaurer
jensmaurer reviewed Dec 14, 2018
View reviewed changes
source/ranges.tex Outdated
@@ -4438,7 +4438,7 @@
constexpr outer_iterator(Parent& parent, iterator_t<Base> current)
requires ForwardRange<Base>;
constexpr outer_iterator(outer_iterator<!Const> i)
requires Const && ConvertibleTo<iterator_t<V>, iterator_t<const V>>;
requires Const && ConvertibleTo<iterator_t<V>, 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.

This looks like a normative change. What's the argument why this is editorial, i.e. implied by normative text elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const V and Base are equivalent, because when Const is true (required earlier in this requires clause), Base is const V:

draft/source/ranges.tex

Lines 4422 to 4423 in a88d02d

using Base = // \expos
conditional_t<Const, const V, V>;

The intention is to be consistent with the other range adaptor iterators/sentinels:

draft/source/ranges.tex

Lines 2309 to 2310 in a88d02d

constexpr iterator(iterator<!Const> i)
requires Const && ConvertibleTo<iterator_t<V>, iterator_t<Base>>;

draft/source/ranges.tex

Lines 2654 to 2655 in a88d02d

constexpr sentinel(sentinel<!Const> i)
requires Const && ConvertibleTo<sentinel_t<V>, sentinel_t<Base>>;

draft/source/ranges.tex

Lines 3518 to 3519 in a88d02d

constexpr sentinel(sentinel<!Const> s)
requires Const && ConvertibleTo<sentinel_t<V>, sentinel_t<Base>>;

draft/source/ranges.tex

Lines 3732 to 3736 in a88d02d

constexpr iterator(iterator<!Const> i)
requires Const &&
ConvertibleTo<iterator_t<V>, iterator_t<Base>> &&
ConvertibleTo<iterator_t<InnerRng>,
iterator_t<iter_reference_t<iterator_t<Base>>>>;

draft/source/ranges.tex

Lines 4027 to 4028 in a88d02d

constexpr sentinel(sentinel<!Const> s)
requires Const && ConvertibleTo<sentinel_t<V>, sentinel_t<Base>>;

Copy link
Contributor

Choose a reason for hiding this comment

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

While the result is equivalent, this change seems to obfuscate the spec by introducing a layer of indirection. I think it would be better to change the other occurrences to use const V instead of 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 agree with you in this case, but am nevertheless hesitant to make those changes.

Copy link
Member

Choose a reason for hiding this comment

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

@JohelEGP, since you're not willing to change "Base" to "const V" in the "other" occurrences, please remove the "[range.split.outer] Use Base for consistency with other adaptors" patch from this pull request (force pushing is fine) and consider opening a new editorial issue. This allows the other two fixes to go in. Since @CaseyCarter is deeply involved with ranges, we should respect his editorial guidance by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. For future referece, @CaseyCarter, should both Base in the following requires clause be changed?

draft/source/ranges.tex

Lines 3732 to 3736 in a88d02d

constexpr iterator(iterator<!Const> i)
requires Const &&
ConvertibleTo<iterator_t<V>, iterator_t<Base>> &&
ConvertibleTo<iterator_t<InnerRng>,
iterator_t<iter_reference_t<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.

I'd say yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

As would I.

@jensmaurer jensmaurer added the changes requested Changes to the wording or approach have been requested and not yet applied. label Dec 17, 2018
jensmaurer
jensmaurer approved these changes Dec 19, 2018
View reviewed changes
Copy link
Member

@jensmaurer jensmaurer left a comment

Choose a reason for hiding this comment

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

The set of changes remaining in this pull request look good. @tkoeppe, I'd suggest to apply.

@jensmaurer jensmaurer added tiny An issue with a small change; with "cwg" label: can be applied editorially after CWG consent. and removed changes requested Changes to the wording or approach have been requested and not yet applied. labels Dec 19, 2018
@tkoeppe
Copy link
Contributor

tkoeppe commented Dec 19, 2018

I wasn't following the discussion; does everyone agree that adding && is_object_v<Pred> is editorial?

@JohelEGP
Copy link
Contributor Author

Yes. Here's the definition which has that part of the requires clause.

draft/source/ranges.tex

Lines 1681 to 1687 in a88d02d

\rSec3[range.filter.view]{Class template \tcode{filter_view}}
\begin{codeblock}
namespace std::ranges {
template<InputRange V, IndirectUnaryPredicate<iterator_t<V>> Pred>
requires View<V> && is_object_v<Pred>
class filter_view : public view_interface<filter_view<V, Pred>> {

@tkoeppe
Copy link
Contributor

tkoeppe commented Dec 19, 2018

Thank you!

@tkoeppe tkoeppe merged commit 31c6cf7 into cplusplus:master Dec 19, 2018
@JohelEGP JohelEGP deleted the range_adaptors branch December 19, 2018 21:48
@JohelEGP JohelEGP mentioned this pull request Dec 19, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CaseyCarter CaseyCarter CaseyCarter left review comments

@jensmaurer jensmaurer jensmaurer approved these changes

Assignees
No one assigned
Labels
tiny An issue with a small change; with "cwg" label: can be applied editorially after CWG consent.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JohelEGP @tkoeppe @CaseyCarter @jensmaurer