Skip to content

Conversation

@dietmarkuehl
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings January 18, 2026 23:14
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements workarounds for a clang compiler issue by modifying the prop deduction guide and refactoring the affine_on implementation to avoid problematic code patterns.

Changes:

  • Modified the prop deduction guide to accept an additional non_assignable parameter with a default value
  • Introduced a new ao_env helper struct to replace the combination of join_env and env{prop{...}}
  • Renamed the env parameter to ev in transform_sender to avoid naming conflicts with the env template

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
include/beman/execution/detail/prop.hpp Updated deduction guide to include third parameter for workaround
include/beman/execution/detail/affine_on.hpp Added ao_env helper struct and refactored code to avoid clang issues

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

#else
if constexpr (::beman::execution::detail::nested_sender_has_affine_on<Sender, Env>)
#endif
{
Copy link

Copilot AI Jan 18, 2026

Choose a reason for hiding this comment

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

Creating a constexpr local variable t and then immediately using it to call affine_on is unusual. If this is a workaround for a clang issue, consider adding a comment explaining why this intermediate variable is necessary instead of directly using child_tag_t{}.affine_on(...) as was done previously.

Suggested change
{
{
// NOTE: Some compilers (e.g. certain Clang versions) have had issues
// when invoking affine_on on a temporary, i.e. child_tag_t{}.affine_on(...).
// Using a named constexpr instance here avoids those issues; do not
// simplify back to a temporary without re-validating compiler support.

Copilot uses AI. Check for mistakes.
::beman::execution::env{::beman::execution::prop{::beman::execution::get_stop_token,
::beman::execution::never_stop_token{}}},
::beman::execution::env{::beman::execution::prop{
::beman::execution::get_stop_token, ::beman::execution::never_stop_token{}, {}}},
Copy link

Copilot AI Jan 18, 2026

Choose a reason for hiding this comment

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

The third parameter {} is passed explicitly to the prop constructor even though the deduction guide provides a default value for the non_assignable parameter. If the default value works correctly, this explicit {} may be unnecessary. Consider removing it to simplify the code and rely on the default parameter, or add a comment explaining why the explicit parameter is required.

Suggested change
::beman::execution::get_stop_token, ::beman::execution::never_stop_token{}, {}}},
::beman::execution::get_stop_token, ::beman::execution::never_stop_token{}}},

Copilot uses AI. Check for mistakes.

template <typename Query, typename Value>
prop(Query, Value) -> prop<Query, ::std::unwrap_reference_t<Value>>;
prop(Query, Value, ::beman::execution::detail::non_assignable = {}) -> prop<Query, ::std::unwrap_reference_t<Value>>;
Copy link

Copilot AI Jan 18, 2026

Choose a reason for hiding this comment

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

The deduction guide now requires passing an extra parameter (the non_assignable object with default value) that doesn't correspond to a user-provided value. While this may work around a clang issue, this creates an inconsistency where users of prop must now be aware of this implementation detail. Consider whether this is the intended workaround, as it exposes the internal non_assignable_ member in the public API of the deduction guide.

Copilot uses AI. Check for mistakes.
@coveralls
Copy link

coveralls commented Jan 18, 2026

Coverage Status

coverage: 92.63% (-0.006%) from 92.636%
when pulling ca4a70e on work-around-clang-issue
into 7d2a2b0 on main.

@dietmarkuehl dietmarkuehl merged commit a098f90 into main Jan 18, 2026
37 checks passed
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.

3 participants