added a proposed resolution for issue 4347

[dietmarkuehl]

No description provided.

[Copilot]

Pull request overview

This PR adds a proposed resolution for issue 4347, which involves restructuring the stop token management in C++ execution's task class template. The resolution moves the stop_source_type and stop_token_type members from the promise_type class to the state class, and introduces a new exposition-only function get-stop-token() to manage stop token initialization more appropriately.

Changes:

• Adds an optional<stop_source_type> member and get-stop-token() function to the task::state class
• Modifies the start() method specification and adds the new get-stop-token() method specification in [task.state]
• Removes token and source members from promise_type in [task.promise]
• Updates get_env() specification to use STATE(*this).get-stop-token() instead of token

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The control flow in this paragraph is unclear. The sentence structure "Otherwise, if source.has_value() is false, let st be..." followed by initialization instructions doesn't properly handle the case when source.has_value() is true. The paragraph should clarify what happens in both branches of the conditional, and should include a return statement or else clause for when source already has a value. Additionally, the lowercase "l" in "let" after the insertion marker suggests a formatting error where part of the original text was incorrectly preserved.

Suggested change

	<ins>-6- <i>Effects</i>: If <code>same_as&lt;decltype(declval&lt;stop_source_type&gt;().get_token()), decltype(get_stop_token(get_env(<i>rcvr</i>)))&gt;</code> is <code>true</code> returns <code>get_stop_token(get_env(<i>rcvr</i>))</code>.
	Otherwise, if <code><i>source</i>.has_value()</code> is <code>false</code>, l</ins><del>L</del>et <code>st</code> be <code>get_stop_token(get_env(<i>rcvr</i>))</code>. Initializes <del><code><i>prom</i>.<i>token</i></code> and <code><i>prom</i>.<i>source</i></code></del><ins><code><i>source</i></code></ins> such that
	<ins>-6- <i>Effects</i>: If <code>same_as&lt;decltype(declval&lt;stop_source_type&gt;().get_token()), decltype(get_stop_token(get_env(<i>rcvr</i>)))&gt;</code> is <code>true</code>, returns <code>get_stop_token(get_env(<i>rcvr</i>))</code>.
	Otherwise, let <code>st</code> be <code>get_stop_token(get_env(<i>rcvr</i>))</code>. If <code><i>source</i>.has_value()</code> is <code>false</code>, initializes <del><code><i>prom</i>.<i>token</i></code> and <code><i>prom</i>.<i>source</i></code></del><ins><code><i>source</i></code></ins> such that</ins>

[dietmarkuehl]

AI got the logic wrong. The structure if specification isn’t ideal, though. If there is a recommendation how to do it better I’m happy to apply it.

[jwakely]

I don't know when the copilot reviews got enabled (probably MS adding it to everything without being asked to) but the other reviews it did were useful.

[dietmarkuehl]

I don't know when the copilot reviews got enabled (probably MS adding it to everything without being asked to) but the other reviews it did were useful.

Yes. The other reviews should be addressed: is there something needed or can this PR be merged. It seems I'd have persmission to merge it but I think using that would appear abusive. Maybe there should be branch protection?

[jwakely]

The AI's suggestion is wrong, but I think there is a problem here.

We have a returns statement inside the Effects: which contradicts the Returns: paragraph.

Can we remove the Returns: paragraph entirely, and then describe it all in the Effects:?

Something like:

Let st be get_stop_token(...).

If same_as<...> is true, returns st. Otherwise, if source.has_value() is false, initializes source such that source->get_token() == st. Then returns source->get_token().

Also, please avoid sub-word edits like <ins>l</ins><del>L</del>et, just replace the whole word: <ins>let</ins><del>Let</del>.

[jwakely]

I think what confused the AI is that the "Initializes source such that ..." part is a continuation of "If source.has_value() is false", but it's written as a separate sentence that isn't conditional on that.

If I understand correctly, the logic is:

if (not source)
  source = st;
return source->get_token();

But the way it's written with multiple sentences and a separate Returns: paragraph doesn't really match that logic.

Maybe another way to say it would be:

If source.has_value() is true, returns source->get_token(). Otherwise, initializes source such that [...], where st is get_stop_token(...), then returns source->get_token().

[jwakely]

Oh right, p7 is not actually a Returns: it just says the word, sorry!

[jwakely]

It does seem confusing to have the first "If ..., returns ..." in the same paragraph as the "initializes source" part, but then the final "Returns ..." in a separate paragraph. The final Returns belongs with the "Otherwise, ..." text, but is physically distant from it.

Maybe add another paragraph after the first sentence of the Effects:, to separate the "types are the same" and "types are not the same" branches more clearly. And then maybe eliminate st completely?

6. Effects: If same_as<decltype(declval<stop_source_type>().get_token()), decltype(get_stop_token(get_env(rcvr)))> is true returns get_stop_token(get_env(rcvr)).
7. Otherwise, if source.has_value() is false, initializes source such that:
   -- source->stop_requested() returns get_stop_token(get_env(rcvr))->stop_requested(); and
   -- source->stop_possible() returns get_stop_token(get_env(rcvr))->stop_possible().
8. Finally, returns source->get_token().

[jwakely]

The AI's suggestion is wrong, but I think there is a problem here.

We have a returns statement inside the Effects: which contradicts the Returns: paragraph.

Can we remove the Returns: paragraph entirely, and then describe it all in the Effects:?

Something like:

Let st be get_stop_token(...).

If same_as<...> is true, returns st. Otherwise, if source.has_value() is false, initializes source such that source->get_token() == st. Then returns source->get_token().

Also, please avoid sub-word edits like <ins>l</ins><del>L</del>et, just replace the whole word: <ins>let</ins><del>Let</del>.

[dietmarkuehl]

OK. I try to phrase it differently. I don’ really have a Returns: clause but the last paragraph does look somewhat like it.

The logic (for type erasing the stop token type) is:

if constexpr (<receiver’s stop token type matches>)
    return <receiver’s stop token>;
else {
    if (_source_.initialized)
        initialize _source_;
    return _source_->get_token();
}

The source shouldn’t exist if the type from the receiver matches.

[jwakely]

Suggested change

	<ins>-5- After that invokes handle.resume().</ins>
	<ins>-5- After that invokes <code><i>handle</i>.resume()</code>.</ins>

[jwakely]

Suggested change

	<del>After that invokes handle.resume().</del>
	<del>After that invokes <code><i>handle</i>.resume()</code>.</del>