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

[unord.req], [fs.path.io] Fix "Effects: Equivalent to" styles #1806

Merged
merged 1 commit into from Nov 25, 2017
Merged

[unord.req], [fs.path.io] Fix "Effects: Equivalent to" styles #1806

merged 1 commit into from Nov 25, 2017

Conversation

JohelEGP
Copy link
Contributor

Some other places that might require change (wording relative to N4700):

  • [string.view.modifiers] p2: Maybe convert to code block, but there are others like this.
  • [string.view.modifiers] p4: Convert to expression style. Unlike the above, this is unique.
  • [mem.poly.allocator.mem] p1: Maybe this was meant to be an Effects: element.

@JohelEGP JohelEGP changed the title Fix "Effects: Equivalent to" styles Fix "Effects: Equivalent to" styles throughout the standard Nov 16, 2017
@jensmaurer
Copy link
Member

This change overlaps with my #1815. We should pick one, but not both.

@jensmaurer jensmaurer mentioned this pull request Nov 17, 2017
Merged
@JohelEGP
Copy link
Contributor Author

Here are a few changed lines not included in #1815. Where both PR change a line, the changes are the same, so they don't conflict. If it's still undesirable to have two open PRs with the same goal, you could include those changes in #1815.

jensmaurer
jensmaurer reviewed Nov 17, 2017
View reviewed changes
source/containers.tex Outdated
@@ -2522,7 +2522,7 @@
& \tcode{void}
& \requires\ \tcode{value_type} shall be \tcode{EmplaceConstructible} into \tcode{X} from \tcode{*i}.\br
\requires \tcode{i} and \tcode{j} are not iterators in \tcode{a}.
Equivalent to \tcode{a.insert(t)} for each element in \tcode{[i,j)}.%
\effects\ Equivalent to \tcode{a.insert(t)} for each element in \tcode{[i,j)}.%
\indextext{unordered associative containers!\idxcode{insert}}%
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 prefer leaving container requirements tables alone; they need a serious overhaul anyway, and it's unclear whether \effects and friends should or should not appear there.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's true, but for the time being, I think this change improves local consistency, so I don't mind it.

@jensmaurer
Copy link
Member

jensmaurer commented Nov 17, 2017 •
edited

Well, in other places, #1815 has more changes, because it also folds \returns into "\effects Equivalent to" where appropriate. The good news is that both pull requests do the same thing where they overlap.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 25, 2017

Please rebase.

@tkoeppe tkoeppe added the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Nov 25, 2017
@JohelEGP
Copy link
Contributor Author

Done.

@tkoeppe tkoeppe removed the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Nov 25, 2017
tkoeppe
tkoeppe reviewed Nov 25, 2017
View reviewed changes
source/containers.tex Outdated
@@ -2509,7 +2509,7 @@
& \requires\ If \tcode{t} is a non-const rvalue expression, \tcode{value_type} shall be
\tcode{MoveInsertable} into \tcode{X}; otherwise, \tcode{value_type} shall be
\tcode{CopyInsertable} into \tcode{X}.\br
\effects\ Equivalent to a.insert(t). Return value is an iterator pointing
\effects\ Equivalent to \tcode{a.insert(t)}. Return value is an iterator pointing
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the trailing backslashes after \effects etc., as they are not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are many occurrences of this for many elements in many sources. Is it okay to make a separate PR for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, leave the existing document alone. It's just that since you're already touching this part, you might as well clean it up, and I don't want you to introduce new cases of this (in the other change). But don't bother changing other existing instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@JohelEGP JohelEGP force-pushed the effects_equivalent_to_fixes branch 2 times, most recently from d84fcb4 to 9becee6 Compare November 25, 2017 22:29
@JohelEGP JohelEGP changed the title Fix "Effects: Equivalent to" styles throughout the standard [unord.req], [fs.path.io] Fix "Effects: Equivalent to" styles Nov 25, 2017
tkoeppe
tkoeppe reviewed Nov 25, 2017
View reviewed changes
source/containers.tex Outdated
@@ -2522,7 +2522,7 @@
& \tcode{void}
& \requires\ \tcode{value_type} shall be \tcode{EmplaceConstructible} into \tcode{X} from \tcode{*i}.\br
\requires \tcode{i} and \tcode{j} are not iterators in \tcode{a}.
Equivalent to \tcode{a.insert(t)} for each element in \tcode{[i,j)}.%
\effects Equivalent to \tcode{a.insert(t)} for each element in \tcode{[i,j)}.%
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, did you test this? You'll probably need a manual line break of sorts here.

Copy link
Contributor Author

@JohelEGP JohelEGP Nov 25, 2017 •
edited

Choose a reason for hiding this comment

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

No, I didn't. Guess I'll setup a proper latex editing environment with live rendering. Fixed.

@tkoeppe tkoeppe merged commit 9dbbf9c into cplusplus:master Nov 25, 2017
@JohelEGP JohelEGP deleted the effects_equivalent_to_fixes branch November 25, 2017 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkoeppe tkoeppe tkoeppe left review comments

@jensmaurer jensmaurer jensmaurer left review comments

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
3 participants
@JohelEGP @jensmaurer @tkoeppe