Skip to content

[task.promise] Clarifications and disambiguations #8134

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
Merged
merged 1 commit into from
Jul 31, 2025
Merged

[task.promise] Clarifications and disambiguations #8134

merged 1 commit into from
Jul 31, 2025
+9 −9

Conversation

tkoeppe
Copy link
Contributor

@tkoeppe tkoeppe commented Jul 30, 2025

Cleans up multiple issues in p16:

  • The name "alloc" was confusingly used both for an exposition-only member and for a locally defined variable. The latter is renamed to "alc".
  • Locally defined variables are just typeset as normal code, not as placeholders.
  • Some commas inserted to separate coordinate subclauses.
  • Replaced "(if any)" with leading "Oherwise", which seems better in order to avoid seemingly providing two conflicting definitions of alc.

Open question: What is "Allocator"?

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 30, 2025

image

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 30, 2025

@Dani-Hub Could you please have a look?

@Dani-Hub
Copy link
Member

@Dani-Hub Could you please have a look?

Hmmh, good question. I think the intention is that it is supposed to be allocator_type (As specified in [task.class] b (2.1) ("allocator_type is Environment::allocator_type if [...]"), that seems also be the allocator_type that the synopsis in [task.promise] is referring to. @dietmarkuehl : Is that correct? If this is not correct I suggest that we should open an LWG issue.

@AlisdairM
Copy link
Contributor

The current wording with its current expos-only identifiers looks correct to me.
I think the issue is that the paragraph is poorly constructed.

If there is no allocator_arg_t, then alloc is this; otherwise alloc is that.

Then, regardless of "if' branch above, PAlloc is this type, and alloc/alloc are used correctly in the text that follows.

So I believe no renaming is required, but the paragraph could be better structured.

@dietmarkuehl
Copy link
Contributor

@Dani-Hub Could you please have a look?

Hmmh, good question. I think the intention is that it is supposed to be allocator_type (As specified in [task.class] b (2.1) ("allocator_type is Environment::allocator_type if [...]"), that seems also be the allocator_type that the synopsis in [task.promise] is referring to. @dietmarkuehl : Is that correct? If this is not correct I suggest that we should open an LWG issue.

Yes, that is correct: [task.class] p2.1 specifies how allocator_type is computed. The Allocator seems to be a left-over from earlier wording which slipped through the wording review.

dietmarkuehl
dietmarkuehl reviewed Jul 30, 2025
View reviewed changes
dietmarkuehl
dietmarkuehl reviewed Jul 30, 2025
View reviewed changes
Copy link
Contributor

@dietmarkuehl dietmarkuehl left a comment

Choose a reason for hiding this comment

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

I'm sorry for the left-over Allocator: it should be allocator_type (defined in [exec.task] p2.1) in all places.

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 30, 2025

@Dani-Hub, @dietmarkuehl could you have another look?

dietmarkuehl
dietmarkuehl approved these changes Jul 31, 2025
View reviewed changes
Copy link
Contributor

@dietmarkuehl dietmarkuehl left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Dani-Hub
Dani-Hub approved these changes Jul 31, 2025
View reviewed changes
Copy link
Member

@Dani-Hub Dani-Hub left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 31, 2025

Thanks, everyone!

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 31, 2025

I reverted the renaming of "alloc" to "alc". I think Alisdair has a point that there is no local ambiguity once the typography has been cleaned up, and the Otherwise rephrasing makes the local definition fairly clear.

@Dani-Hub
Copy link
Member

Agreed

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 31, 2025

I'm also changing "Then X is" to "Let X be" for the PAlloc sentence, which (I think) clears up Alisdairs other complaint.

@tkoeppe
[task.promise] Fixes and clarifications in "operator new" spec
09a341f 
* Change erroneous "Allocator" to "allocator_type". This was
  a mistake in the paper (a leftover from a previous revision).
* Locally defined variables are just typeset as normal code,
  not as placeholders.
* Some commas inserted to separate coordinate subclauses.
* Replaced "(if any)" with leading "Oherwise", which seems better
  in order to avoid seemingly providing two conflicting definitions
  of alloc.
* Replaced "Then PAlloc is ..." with "Let PAlloc be ...", which
  replaces the (causal vs temporal) ambiguous "then" with the
  far more common "let" expression.
@tkoeppe
Copy link
Contributor Author

tkoeppe commented Jul 31, 2025

image

@tkoeppe tkoeppe requested review from dietmarkuehl and Dani-Hub July 31, 2025 10:48
@tkoeppe tkoeppe merged commit e15c095 into cplusplus:main Jul 31, 2025
2 checks passed
@tkoeppe tkoeppe deleted the promise_alc branch July 31, 2025 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dani-Hub Dani-Hub Dani-Hub approved these changes

@dietmarkuehl dietmarkuehl Awaiting requested review from dietmarkuehl

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tkoeppe @Dani-Hub @AlisdairM @dietmarkuehl