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

Use "denotes a type" in utilities library clause #2678

Merged
merged 10 commits into from Aug 18, 2022
Merged

Use "denotes a type" in utilities library clause #2678

merged 10 commits into from Aug 18, 2022

Conversation

jwakely
Copy link
Member

@jwakely jwakely commented Feb 22, 2019

This replaces #2354, in line with the editorial meeting decision.

@jwakely jwakely mentioned this pull request Feb 22, 2019
Closed
zygoloid
zygoloid reviewed Mar 15, 2019
View reviewed changes
source/utilities.tex Outdated Show resolved Hide resolved
@zygoloid zygoloid added the changes requested Changes to the wording or approach have been requested and not yet applied. label Mar 15, 2019
@jwakely
Copy link
Member Author

jwakely commented Mar 15, 2019

I've rebased it on master, and made the requested changes.

@jensmaurer jensmaurer removed the changes requested Changes to the wording or approach have been requested and not yet applied. label Mar 15, 2019
zygoloid
zygoloid requested changes Mar 15, 2019
View reviewed changes
source/utilities.tex Outdated Show resolved Hide resolved
source/utilities.tex Outdated Show resolved Hide resolved
source/utilities.tex Outdated Show resolved Hide resolved
@zygoloid zygoloid added the changes requested Changes to the wording or approach have been requested and not yet applied. label Mar 15, 2019
@jwakely jwakely requested a review from zygoloid March 16, 2019 23:37
@jensmaurer jensmaurer removed the changes requested Changes to the wording or approach have been requested and not yet applied. label Apr 16, 2019
@jwakely
Copy link
Member Author

jwakely commented Apr 23, 2019

Rebased and a new commit added to make [refwrap.unwrapref] use "denotes the type" too.

CaseyCarter
CaseyCarter reviewed Apr 23, 2019
View reviewed changes
source/utilities.tex Outdated
otherwise it is \tcode{T}.
the member typedef \tcode{type} of \tcode{unwrap_reference<T>}
denotes the type \tcode{X\&},
otherwise \tcode{type} denotes the type \tcode{T}.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's very much clear here that T and X& are types, and that a "member typedef" must denote a type; do we really need "the type" in the two lines above to remind us? "...the member typedef type of unwrap_reference<T> denotes X&, otherwise type denotes T" seems perfectly clear and unambiguous to me.

Ditto 17836-37, 17843-44, 17853-54, 17876, 17893, 17970-71, 17998, 18056, 18063-64.

Copy link
Member Author

Choose a reason for hiding this comment

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

The decision recorded at #2354 (comment) was to say "denotes the type" but I agree that saying it's a type is redundant in many cases.

source/utilities.tex Outdated Show resolved Hide resolved
source/utilities.tex Outdated Show resolved Hide resolved
source/utilities.tex Outdated Show resolved Hide resolved
@jensmaurer jensmaurer added the changes requested Changes to the wording or approach have been requested and not yet applied. label May 30, 2019
@jensmaurer jensmaurer added the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Dec 18, 2019
@jwakely
Copy link
Member Author

jwakely commented Apr 20, 2022

Rebased and addressed Casey's comments.

@jwakely jwakely removed needs rebase The pull request needs a git rebase to resolve merge conflicts. changes requested Changes to the wording or approach have been requested and not yet applied. labels Apr 20, 2022
jensmaurer
jensmaurer approved these changes Apr 20, 2022
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.

I like it.

@tkoeppe
Copy link
Contributor

tkoeppe commented Apr 22, 2022

@jwakely Are all of @zygoloid comments addressed? And do you want to keep all the individual commits? If so, please go ahead and merge!

@jwakely
Copy link
Member Author

jwakely commented Apr 22, 2022

@jwakely Are all of @zygoloid comments addressed?

Actually I think #2678 (comment) got lost when I rebased, because that wording moved to Annex D and I didn't reapply it correctly. I'll push another commit and check if anything else got lost.

@jwakely
Copy link
Member Author

jwakely commented Apr 22, 2022

I'll also apply the change requested at #2678 (comment) to remove_const and remove_volatile, not just to remove_cv.

This was a pain to rebase, because every single change moved from utilities.tex to either meta.tex or future.tex so had to be manually resolved.

@tkoeppe
Copy link
Contributor

tkoeppe commented Apr 22, 2022

Oh, yes, sorry about that -- thanks a lot!

@jwakely
Copy link
Member Author

jwakely commented Apr 22, 2022

OK, I think all changes are present and correct.

@jwakely
Copy link
Member Author

jwakely commented Apr 22, 2022

I don't think we need to keep the individual commits, I think squash+merge makes more sense. But I can rebase it to remove the FIXUP ones if you want the individual commits.

@tkoeppe
Copy link
Contributor

tkoeppe commented Apr 22, 2022

Yes, if a smaller number, but still more than one, makes sense, then a bit of grouping and separation is also nice.

@jwakely
Copy link
Member Author

jwakely commented Apr 22, 2022

OK, cleaned up a bit and rebased. I'll leave this for a day or so for other comments and then merge.

@tkoeppe
Copy link
Contributor

tkoeppe commented Aug 18, 2022

I actually like the individual commits. Very tidy.

@tkoeppe tkoeppe merged commit e412ba9 into cplusplus:main Aug 18, 2022
@jwakely jwakely deleted the denotes branch September 24, 2022 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jensmaurer jensmaurer jensmaurer approved these changes

@zygoloid zygoloid Awaiting requested review from zygoloid

@CaseyCarter CaseyCarter Awaiting requested review from CaseyCarter

Assignees

@tkoeppe tkoeppe

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jwakely @tkoeppe @zygoloid @CaseyCarter @jensmaurer