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] Replace Base with const V for clarity #2605

Closed
wants to merge 1 commit into from
Closed

[range.adaptors] Replace Base with const V for clarity #2605

wants to merge 1 commit into from

Conversation

JohelEGP
Copy link
Contributor

See discussion at #2596 (comment).

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.

Works for me.

@jensmaurer
Copy link
Member

jensmaurer commented Jan 7, 2019 •
edited

LWG 3181 says we must use "Base" here, not "const V", to correctly handle the template instantiation of the enclosing class. @CaseyCarter, have you changed your opinion since you approved these changes?

@jensmaurer jensmaurer added lwg Issue must be reviewed by LWG. not-editorial Issue is not deemed editorial; the editorial issue is kept open for tracking. labels Jan 7, 2019
@CaseyCarter
Copy link
Contributor

@CaseyCarter, have you changed your opinion since you approved these changes?

Yes, and I've since changed it back again after running a canonical example past the core reflector. I currently believe these changes are valid and the concept implementations are buggy. Sorry for the confusion - it can be challenging to determine which issues are actual problems with the spec and which are bugs in the implementations.

@JohelEGP
Copy link
Contributor Author

JohelEGP commented Jan 7, 2019

@CaseyCarter, have you changed your opinion since you approved these changes?

[...] I currently believe these changes are valid and the concept implementations are buggy. [...]

That's right. Using requires clauses doesn't require you to do the same workaround of introducing a dummy template parameter defaulted to some outer one for the sake of delayed instantiation to apply SFINAE.

@jensmaurer jensmaurer removed not-editorial Issue is not deemed editorial; the editorial issue is kept open for tracking. lwg Issue must be reviewed by LWG. labels Jan 7, 2019
zygoloid
zygoloid reviewed Jan 19, 2019
View reviewed changes
source/ranges.tex
@@ -2307,7 +2307,7 @@
iterator() = default;
constexpr iterator(Parent& parent, iterator_t<Base> current);
constexpr iterator(iterator<!Const> i)
Copy link
Member

Choose a reason for hiding this comment

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

It'd be clearer to use iterator<false> here. Oh, we can't do that because that'd be ill-formed when instantiating with Const = false. Yuck, what a language!

@zygoloid
Copy link
Member

I believe this is technically not editorial: the precise spelling of the requires clause is detectable by user code, based on partial ordering in derived classes that inherit the constructors of this base class and add overloads.

@zygoloid zygoloid added lwg Issue must be reviewed by LWG. not-editorial Issue is not deemed editorial; the editorial issue is kept open for tracking. labels Jan 19, 2019
@jensmaurer jensmaurer added the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Jan 19, 2019
@JohelEGP
Copy link
Contributor Author

JohelEGP commented Dec 4, 2019

https://wg21.link/P1983 will take care of this.

@JohelEGP JohelEGP closed this Dec 4, 2019
@JohelEGP JohelEGP deleted the ranges_iter_sent_conv_to_const branch December 4, 2019 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zygoloid zygoloid zygoloid left review comments

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@jensmaurer jensmaurer jensmaurer approved these changes

Assignees
No one assigned
Labels
lwg Issue must be reviewed by LWG. needs rebase The pull request needs a git rebase to resolve merge conflicts. not-editorial Issue is not deemed editorial; the editorial issue is kept open for tracking.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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