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

LWG Poll 5: P2160R1 Locks lock lockables #4520

Merged
merged 1 commit into from Mar 9, 2021
Merged

LWG Poll 5: P2160R1 Locks lock lockables #4520

merged 1 commit into from Mar 9, 2021

Conversation

burblebee
Copy link
Contributor

@burblebee burblebee commented Feb 25, 2021 •
edited by jensmaurer

[thread.req.lockable.general] Use "or" instead of "and" (a lock can't be both
a shared lock and a shared lock).
[thread.req.lockable.shared][thread.req.lockable.shared.timed] Fix punctuation
to be consistent.

Fixes #4510.
Fixes cplusplus/papers#871

Issues/Questions:
*[thread.req.lockable.shared.timed]p2 uses "has not been acquired" while [thread.req.lockable.shared]p2 uses "shall not have been acquired". I believe these two should use the same wording. Which is correct?

@burblebee
Copy link
Contributor Author

Why did this build fail?

JohelEGP
JohelEGP suggested changes Feb 25, 2021
View reviewed changes
source/threads.tex Outdated Show resolved Hide resolved
source/threads.tex Show resolved Hide resolved
source/threads.tex Outdated Show resolved Hide resolved
source/threads.tex Outdated Show resolved Hide resolved
@JohelEGP
Copy link
Contributor

Why did this build fail?

The underscores in https://github.com/cplusplus/draft/runs/1974880075#step:4:2160 are non-ASCII.

jensmaurer
jensmaurer requested changes Feb 25, 2021
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.

The commit description says "[thread.req.lockable.general] Use "or" instead of "and" (a lock can't be both a shared lock and a shared lock)."

Note the repeated "shared lock". That doesn't make sense. I'd suggest you force-push after fixing. Also, please address @JohelEGP's comments.

@jensmaurer jensmaurer added this to the post-2021-02 milestone Feb 25, 2021
@jensmaurer
Copy link
Member

The build failed because, apparently, you did some cut&paste and that employed non-ASCII characters (use Details / view raw logs):

2021-02-25T00:14:12.6278370Z threads.tex:391:\tcode{rel_­time} denotes a value of a specialization of duration, and <--- non-ASCII character
2021-02-25T00:14:12.6279673Z threads.tex:392:\tcode{abs_­time} denotes a value of a specialization of \tcode{time_­point}). <--- non-ASCII character
2021-02-25T00:14:12.6281093Z threads.tex:402:the relative timeout\iref{thread.req.timing} specified by \tcode{rel_­time}. <--- non-ASCII character
2021-02-25T00:14:12.6282221Z threads.tex:403:The function will not return within the timeout specified by \tcode{rel_­time} <--- non-ASCII character
2021-02-25T00:14:12.6283351Z threads.tex:421:the absolute timeout\iref{thread.req.timing} specified by \tcode{abs_­time}. <--- non-ASCII character
2021-02-25T00:14:12.6284485Z threads.tex:422:The function will not return before the timeout specified by \tcode{abs_­time} <--- non-ASCII character

@burblebee
Copy link
Contributor Author

burblebee commented Feb 26, 2021 •
edited

The underscores in https://github.com/cplusplus/draft/runs/1974880075#step:4:2160 are non-ASCII.

Thanks! How did you figure that out?
Just saw Jens's answer to this... Is there any way we can flag the build output "non-ASCII character" in red so it stands out?

@JohelEGP
Copy link
Contributor

I googled "detect non-ascii" and pasted that onto https://pages.cs.wisc.edu/~markm/ascii.html.

@JohelEGP
Copy link
Contributor

Just saw Jens's answer to this... Is there anyway we can flag the build output "non-ASCII character" in red so it stands out?

Yes. By using https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions#setting-an-error-message. I'm not sure whether tools/check.sh would need to be converted to an action first.

@JohelEGP JohelEGP mentioned this pull request Feb 26, 2021
Closed
jensmaurer
jensmaurer requested changes Feb 27, 2021
View reviewed changes
source/threads.tex Outdated Show resolved Hide resolved
jensmaurer
jensmaurer requested changes Mar 3, 2021
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 think I found a missed edit.

source/threads.tex Show resolved Hide resolved
@burblebee @tkoeppe
P2160R1 Locks lock lockables
d64b1fa 
* [thread.req.lockable.general] Use bullets with "or" instead of "and"
  (a lock can't be both a shared lock and a non-shared lock).

* [thread.req.lockable.shared, thread.req.lockable.shared.timed] Fix
  punctuation to be consistent.
@tkoeppe tkoeppe merged commit 796fa8a into master Mar 9, 2021
@jensmaurer jensmaurer deleted the motions-2021-02-lwg-5 branch June 14, 2021 21:27
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

@timsong-cpp timsong-cpp timsong-cpp left review comments

@JohelEGP JohelEGP JohelEGP requested changes

@jensmaurer jensmaurer jensmaurer requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
post-2021-02
Development

Successfully merging this pull request may close these issues.

[2021-02 LWG Motion 5] P2160R1 Locks lock lockables P2160 Locks lock lockables (wording for LWG 2363)
5 participants
@burblebee @JohelEGP @jensmaurer @tkoeppe @timsong-cpp