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.chunk.overview,range.slide.overview] Correct the italics of N/M in the overview #5500

Merged
merged 2 commits into from Aug 20, 2022
Merged

[range.chunk.overview,range.slide.overview] Correct the italics of N/M in the overview #5500

merged 2 commits into from Aug 20, 2022

Conversation

hewillk
Copy link
Contributor

@hewillk hewillk commented May 29, 2022

No description provided.

jensmaurer
jensmaurer requested changes May 29, 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 don't think we want that. N is a template parameter and thus is at the C++ source code level, and thus should be code font, not math font.

For the [range.slide.overview] changes regarding "M", this seems to be purely a mathematical placeholder not actually represented in C++ source code. Changing this to math font is fine, but maybe we want lowercase, too, to make the difference more apparent?

@jensmaurer jensmaurer added the changes requested Changes to the wording or approach have been requested and not yet applied. label May 29, 2022
@hewillk
Copy link
Contributor Author

hewillk commented May 29, 2022 •
edited

I don't think we want that. N is a template parameter and thus is at the C++ source code level, and thus should be code font, not math font.

This change is to be consistent with overview section in [range.take.overview], [range.drop.overview], [range.elements.overview], [range.zip.transform.overview], [range.adjacent.overview], [range.adjacent.transform.overview], which all use math fonts.

Maybe they need to change too.

@tkoeppe
Copy link
Contributor

tkoeppe commented Aug 18, 2022

@jensmaurer: is N actually a template parameter here? It seems to be used quite similarly to how drop_view works. What's worse, there's a second, unrelated N in [range.slide.overview]p2.

@JohelEGP
Copy link
Contributor

N is only a template parameter in elements_view, adjacent_view and adjacent_transform_view. But they use an undefined N in the first paragraph of their overview.

@jensmaurer
Copy link
Member

No, sorry, N is not a template parameter, but just some value. In the second paragraph, we say "Given subexpressions \tcode{E} and \tcode{N},"

So, this change for the first paragraph (where we talk about the abstract concept) actually looks good.

@wg21bot wg21bot added the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Aug 19, 2022
@tkoeppe
Copy link
Contributor

tkoeppe commented Aug 19, 2022

@hewillk Can you please rebase?

@tkoeppe tkoeppe merged commit d2ad001 into cplusplus:main Aug 20, 2022
@tkoeppe tkoeppe 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 Aug 20, 2022
@hewillk hewillk deleted the main-N branch August 20, 2022 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jensmaurer jensmaurer jensmaurer requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@hewillk @tkoeppe @JohelEGP @jensmaurer @wg21bot