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

[atomics.ref.ops], [atomics.types.operations], [util.smartptr.atomic.… #6518

Conversation

Dani-Hub
Copy link
Member

…shared], [util.smartptr.atomic.weak], [atomics.flag]: Reword Preconditions on memory_order values in a positive form

@Dani-Hub
Copy link
Member Author

Dani-Hub commented Aug 26, 2023

I need some advice how to fix the "overfull \hbox" errors. It seems to say that the resulting text would need too much space in horizontal direction. OK, let's look at the first case "lines 3154--3160": How is it possible to edit the text to make it even smaller?

@JohelEGP
Copy link
Contributor

For reviewing, it's not helpful that the PDF isn't uploaded because the output check failed.

@Dani-Hub
Copy link
Member Author

For reviewing, it's not helpful that the PDF isn't uploaded because the output check failed.

I suspect this is not something I can fix, right?

Regarding my text changes: I tried to follow the pre-existing line-break structure. Maybe I shouldn't and I should try to provide each paragraph as filled line with breaks at around 70 characters?

@JohelEGP
Copy link
Contributor

I need some advice how to fix the "overfull \hbox" errors.

If you built it locally, you can look at which syllable a word overfills.
You can guess where the margin is by looking at the surrounding text.
Then you can just add a \- in front of the syllable closest to the margin that isn't part of the overfill.

@Dani-Hub
Copy link
Member Author

If you built it locally, you can look at which syllable a word overfills.

Hmmh, I'm on Windows and I'm not sure how to setup my infrastructure to create the pdf locally.

If there is no other way I need to experiment with my PULL request.

@JohelEGP
Copy link
Contributor

I suspect this is not something I can fix, right?

I don't think you need to, as it is a separate problem.

Regarding my text changes: I tried to follow the pre-existing line-break structure. Maybe I shouldn't and I should try to provide each paragraph as filled line with breaks at around 70 characters?

The source line breaks are independent of the rendered text.
https://github.com/cplusplus/draft/wiki/Specification-Style-Guidelines#latex-source-code-formatting says

Use semantic line breaks in LaTeX source code: http://sembr.org/ and http://rhodesmill.org/brandon/2012/one-sentence-per-line/

which I wish was more strictly applied.
Those sources explain the difference well.

@JohelEGP
Copy link
Contributor

To get the PDF uploaded,
you can edit https://github.com/cplusplus/draft/blob/main/.github/workflows/check.yml
and swap the last two steps ("check-output.sh" and "upload PDF") at the end of the file.

@Dani-Hub
Copy link
Member Author

To get the PDF uploaded, you can edit https://github.com/cplusplus/draft/blob/main/.github/workflows/check.yml and swap the last two steps ("check-output.sh" and "upload PDF") at the end of the file.

But isn't that a setting that should be independently set from my PULL request?

@Dani-Hub Dani-Hub force-pushed the #Issue-6511-What-if-the-supplied-arguments-does-not-denote-any-enumerator branch 2 times, most recently from 4f35db3 to cdd3338 Compare August 26, 2023 17:19
@@ -3146,8 +3144,8 @@
\begin{itemdescr}
\pnum
\expects
\tcode{order} is
neither \tcode{memory_order::release} nor \tcode{memory_order::acq_rel}.
\tcode{order} is either \tcode{memory_order::relaxed}, \tcode{memory_order::consume},
Copy link
Member

Choose a reason for hiding this comment

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

Can we harmonize this to read "The order argument is ..." (see next change)

Or maybe the other way round.

Copy link
Member Author

Choose a reason for hiding this comment

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

I harmonized it now as of your first suggestion.

@jensmaurer
Copy link
Member

To get the PDF uploaded, you can edit https://github.com/cplusplus/draft/blob/main/.github/workflows/check.yml and swap the last two steps ("check-output.sh" and "upload PDF") at the end of the file.

But isn't that a setting that should be independently set from my PULL request?

No. In general, I think we can expect someone editing the draft to be able to build locally before uploading. And we only want "good" artifacts published from our continuous integration pipeline.

@Dani-Hub Dani-Hub force-pushed the #Issue-6511-What-if-the-supplied-arguments-does-not-denote-any-enumerator branch from cdd3338 to 2b6150c Compare August 27, 2023 07:42
\tcode{memory_order::consume},
\tcode{memory_order::acquire}, nor
\tcode{memory_order::acq_rel}.
The \tcode{order} argument is either \tcode{memory_order::relaxed},
Copy link
Member

Choose a reason for hiding this comment

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

Strictly speaking, "order" is not an argument, but the name of a parameter.

Copy link
Member Author

Choose a reason for hiding this comment

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

So do you wish me to replace "argument" by "parameter"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to perform this replacement

\tcode{memory_order::consume},
\tcode{memory_order::acquire}, nor
\tcode{memory_order::acq_rel}.
The \tcode{order} argument is either \tcode{memory_order::relaxed},
Copy link
Member

Choose a reason for hiding this comment

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

I believe "either" in English preferentially refers to a choice of two things. We have three things here.
I'd suggest to simply drop "either"; I don't think it adds anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK.

…shared], [util.smartptr.atomic.weak], [atomics.flag]: Reword Preconditions on memory_order values in a positive form and harmonize wording style, also replace 'argument' by 'parameter'.
@Dani-Hub Dani-Hub force-pushed the #Issue-6511-What-if-the-supplied-arguments-does-not-denote-any-enumerator branch from 2b6150c to 6467e1e Compare August 27, 2023 12:19
@jensmaurer
Copy link
Member

I've redone the patch: #6519

It keeps the semantic linebreaks and removes "The argument/parameter" fluff entirely, which is consistent with at least half of the occurrences.

@Dani-Hub
Copy link
Member Author

I've redone the patch: #6519

Thanks for your help.

@JohelEGP
Copy link
Contributor

JohelEGP commented Aug 27, 2023

To get the PDF uploaded, you can edit https://github.com/cplusplus/draft/blob/main/.github/workflows/check.yml and swap the last two steps ("check-output.sh" and "upload PDF") at the end of the file.

But isn't that a setting that should be independently set from my PULL request?

No. In general, I think we can expect someone editing the draft to be able to build locally before uploading. And we only want "good" artifacts published from our continuous integration pipeline.

A fine alternative if one doesn't want to build it locally, or can't, is to do it in one's own fork.
Just enable GitHub Actions there.
In case the branch already has a PR for origin,
branch again to create a playground that doesn't make noise on that PR for origin,
then, once the PDF looks good, hard-reset the PR's branch to its rebranch and force-push.

@JohelEGP
Copy link
Contributor

This can be closed, as it's subsumed by the merged #6519.

@jensmaurer jensmaurer closed this Aug 29, 2023
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

3 participants