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.

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

Further cherry-picks for the c++20 branch #4225

Merged
merged 5 commits into from Sep 23, 2020

Conversation

tkoeppe
Copy link
Contributor

@tkoeppe tkoeppe commented Sep 23, 2020

These are changes that either have no effect but decrease the diff between branches, or that are improvements to the overall quality of the final document.

Please discuss.

See #4220 (comment) for a list of candidate commits.

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Sep 23, 2020

Further suggestions:

  • "Remove superfluous final \rowsep or \hline in tables."
  • "Add missing 'friend' for operator-."

Copy link
Member

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

I think I'm fine with all of these -- though I don't feel motivated to take some of them, I'm also not opposed. I'm mainly just worried about the sticker shock of people seeing hundreds of changes between DIS and IS, even if many of them are trivial, but we've probably already crossed that bridge.

@@ -728,7 +728,7 @@
with the same access control\iref{class.access} and
non-zero size\iref{intro.object}
are allocated so that later
members have higher addresses within a class object.
members have higher addresses within a class object\iref{expr.rel}.
Copy link
Member

Choose a reason for hiding this comment

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

Hm, [expr.rel] says that pointers to them compare greater, it doesn't say anything about "higher addresses" or allocation order. I don't think we actually guarantee anything about field layout order. (A weird implementation could presumably make lower addresses compare greater.)

I'm not sure the footnote makes this worse, though it might make the problem more obvious, because we're pointing directly to where we don't say the thing in question. I'm not sure I see motivation to add this xref for C++20.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, I'll just remove the commit!

Comment on lines 2826 to 2827
postfix-expression \opt{\terminal{.} \terminal{template}} id-expression\br
postfix-expression \opt{\terminal{->} \terminal{template}} id-expression\br
postfix-expression \terminal{.} \opt{\terminal{template}} id-expression\br
postfix-expression \terminal{->} \opt{\terminal{template}} id-expression\br
Copy link
Member

Choose a reason for hiding this comment

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

I omitted this one because it should make no difference to the rendered output.

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, that was precisely a reason in favour for me in order to keep diffs down, but I realize that it makes the Editor's Report more awkward, so I'll remove this and the next commit.

source/lex.tex Outdated
single quote & \tcode{'} & \tcode{\textbackslash\tcode{'}} \\
double quote & \tcode{"} & \tcode{\textbackslash\tcode{"}} \\
single quote & \tcode{'} & \tcode{\textbackslash '} \\
double quote & \tcode{"} & \tcode{\textbackslash "} \\
Copy link
Member

Choose a reason for hiding this comment

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

I omitted this one because it should make no difference to the rendered output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, see above, removing.

@burblebee
Copy link
Contributor

As for what else from the list that should be added...

These seem like harmless yet nice to have consistency improvements:

-[tab:atomic.types.pointer.comp] Fix column captions. (#4137)
-[string.view.template] Wrap synopsis in its namespace (#4038)
-[class.temporary] Omit hyphen from "trivially copyable" (#4002)
-[common.iter.cust] Pluralize subclause heading (#3995)
-[meta.trans.other] "C++ object type" is overly precise (#3969)
-[over.ics.ref] Remove erroneous capitalization (#3972)

Several others are changes to the library for which I don't have an
opinion on. Perhaps Casey should review the list?

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Sep 23, 2020

I remove the unobservable commits and the one with the questionable xref, and I added the commits for the extraneous table hrules and the missing friends. If this is OK now, I'd like to merge this and then restart a PR with burblebee's suggestions.

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Sep 23, 2020

@CaseyCarter: could you please review the suggestions in #4225 (comment)?

@jwakely
Copy link
Member

jwakely commented Sep 23, 2020

I assume Dawn meant to get Casey to review the rest of the list that affects the library, not the ones in #4225 (comment)

Anyway, here's my review of the ones in that comment (green where I feel my opinion counts, i.e. library wording):

-[tab:atomic.types.pointer.comp] Fix column captions. (#4137) ✔️
-[string.view.template] Wrap synopsis in its namespace (#4038) ✔️
-[class.temporary] Omit hyphen from "trivially copyable" (#4002) ✅
-[common.iter.cust] Pluralize subclause heading (#3995) ✔️
-[meta.trans.other] "C++ object type" is overly precise (#3969) ✔️
-[over.ics.ref] Remove erroneous capitalization (#3972) ✅

Copy link
Member

@jwakely jwakely left a comment

Choose a reason for hiding this comment

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

None of these seem important to fix, but they are all improvements and so nice to have.

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Sep 23, 2020

You're right, that's probably how it was meant! OK, I'll merge this now and reopen a PR for additional discussions. Thank you!

@tkoeppe tkoeppe merged commit d625776 into cplusplus:c++20 Sep 23, 2020
@burblebee
Copy link
Contributor

burblebee commented Sep 23, 2020

I assume Dawn meant to get Casey to review the rest of the list that affects the library, not the ones in #4225 (comment)

Yes, I meant for Casey (or someone from lib) to review #4220 (comment). But given Richard's concerns, I say we stop after pr4227.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants