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

[class] Split paragraph defining standard-layout class #2066

Merged
merged 1 commit into from Jul 2, 2018

Conversation

jwakely
Copy link
Member

@jwakely jwakely commented May 11, 2018

This paragraph seems excessively long:

spectacle tj2604

@@ -201,6 +202,7 @@
\item If \tcode{X} is a non-class, non-array type, the set $M(\mathtt{X})$ is empty.
\end{itemize}

\pnum
\begin{note} $M(\mathtt{X})$ is the set of the types of all non-base-class subobjects
that may be at a zero offset in \tcode{X}. \end{note}

Copy link
Contributor

Choose a reason for hiding this comment

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

@jwakely: Please remove this secret intra-paragraph paragraph break here.

@tkoeppe
Copy link
Contributor

tkoeppe commented Jun 27, 2018

Actually, I'm not so sure about this split: M(X) is only defined inside this single paragraph at the moment. With your change, we would leak this expository device into the whole-subclause scope, if you see what I mean.

@jensmaurer
Copy link
Member

This is recent CWG text and I concur with @tkoeppe's view that M(X) is local expository device that we should not spread more.

@jwakely
Copy link
Member Author

jwakely commented Jun 27, 2018

I see what you mean, but the current formatting is ... let's just say it leaves a lot to be desired.

Why do we say "(defined below)" if it's right there in the next sentence? We already say "defined as follows" in that next sentence, how many times do we need a teaser trailer for that definition?

Why do we have that "M(X) is defined as follows" floating in the middle of the list, unattached to any single bullet?

Why is (8.8) a continuation of the same list, if it only applies to (8.7)? Wouldn't the bullets defining M(X) it be better as a nested list, e.g. (8.7.1), (8.7.2) and so on? I know we can't do that with our current itemize env, but here's what it looks like using enumerate instead (rebased on master, so p8 is now p9):

std-layout-list

@jwakely
Copy link
Member Author

jwakely commented Jun 27, 2018

(That doesn't change the fact it's excessively long, but I think it improves the structure)

@jwakely
Copy link
Member Author

jwakely commented Jun 27, 2018

Correction, the example doesn't need to be nested under bullet 7, it applies to the whole paragraph:

std-layout-list

The example could even be its own paragraph with a new \pnum:

std-layout-list2

@jensmaurer
Copy link
Member

@jwakely, the "enumerate" stuff is a regression, not an improvement. I'm ok with a nested "itemize" to define M(S). And when you do that, the "(defined below)" should really go. Asking @zygoloid for an opinion.

@jensmaurer jensmaurer added the decision-required A decision of the editorial group (or the Project Editor) is required. label Jun 27, 2018
@tkoeppe
Copy link
Contributor

tkoeppe commented Jun 27, 2018

I like the last version, with the M(X) part of the same paragraph where it's used, and the example separate.

I'd phrase the relevant item as "has no element of the set M(S) of types as a base class, where M(X) is defined as follows:".

@tkoeppe
Copy link
Contributor

tkoeppe commented Jun 27, 2018

Or even:

has no element of the set M(S) of types as a base class, where for any type X, the set M(X) is defined as follows. [Note: M(X) is the set ... zero offset in X -- end note.]".

@jwakely
Copy link
Member Author

jwakely commented Jun 27, 2018

I'm not suggesting we actually use enumerate, obviously we don't want numbered bullets. When I tried a nested itemize it didn't number the nested bullets properly so I just used enumerate for argument's sake. But I see what I did wrong now (I still had the new \pnum after bullet (8.7)). Here's what I meant , with nested list, example in a new paragraph, and no "(defined below)":

std-layout-list3

We could even join the two sentences in bullet 7:
std-layout-list3a

Or:
std-layout-list3b

@jwakely
Copy link
Member Author

jwakely commented Jun 27, 2018

@tkoeppe my edits crossed with your comments, so I'm not sure which "last version" you mean now :-)

I won't edit it again.

@tkoeppe
Copy link
Contributor

tkoeppe commented Jun 27, 2018

I mean the last screenshot in #2066 (comment). But what you have now is equivalent for the purpose of the stuff I was talking about.

@jensmaurer
Copy link
Member

This is a drive-by, but we really should say what X is in M(X): "For a type X, M(X) is defined as follows..."

@tkoeppe
Copy link
Contributor

tkoeppe commented Jun 27, 2018

@jensmaurer: I just suggested that above :-)

@zygoloid
Copy link
Member

+1 to something in the direction of @tkoeppe's suggestion:

has no element of the set M(S) of types as a base class, where for any type X, the set M(X) is defined as follows. [Note: M(X) is the set ... zero offset in X -- end note.]".

Move the note before the definition of M(X).
Use a nested list for the definition of M(X).
Move the example to a new paragraph.
@jwakely
Copy link
Member Author

jwakely commented Jun 28, 2018

I've just pushed a new commit that results in:

std-layout-list4

@tkoeppe
Copy link
Contributor

tkoeppe commented Jun 28, 2018

Oh, can we fold the footnote into the note?

@zygoloid
Copy link
Member

zygoloid commented Jul 2, 2018

The footnote is a somewhat imprecise rationale for the rule, so I think it would need quite a lot of massaging if we wanted to promote it to a note. Maybe we should consider removing it. But I think this PR is already a significant improvement, so I think we should merge it as-is and leave the footnote for future cleanup.

@zygoloid zygoloid merged commit 3fe46c7 into cplusplus:master Jul 2, 2018
@jwakely jwakely deleted the std-layout-class-paras branch July 2, 2018 22:00
@jensmaurer jensmaurer removed the decision-required A decision of the editorial group (or the Project Editor) is required. label Oct 11, 2019
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

4 participants