Replace C-style cast with move() in let* algos

[ccotter]

No description provided.

[ispeters]

Looks great except that we need to keep stateFactory_ in case it's a stateful callable.

[ispeters]

Somehow this has broken the MSVC build. I wonder if Source is sometimes an lvalue reference.

[ispeters]

Yup; line 324 instantiates an operation with an lvalue ref Source if the Sender argument is an lvalue ref. This is a std::forward.

However, most of the operation state types that I've written myself have a templated constructor that takes a forwarding reference to the Sender, rather than templating the whole type on a forwarding reference. It seems like there'd be less duplicated code in the resulting binary if we could follow that pattern.

[ispeters]

Looks good, but I've got questions about what clang-tidy does and does not like.

[ispeters]

Does clang-tidy complain about the following code?

template <typename Done2, typename Receiver2>
explicit type(Source&& source, Done2&& done, Receiver2&& dest)
    noexcept(std::is_nothrow_move_constructible_v<Receiver> &&
             std::is_nothrow_move_constructible_v<Done> &&
             is_nothrow_connectable_v<Source, source_receiver>)
: done_((Done2&&)done)
, receiver_((Receiver2&&)dest)
{
  unifex::activate_union_member_with(sourceOp_, [&] {
      return unifex::connect(std::forward<Source>(source), source_receiver{this});
    });
  startedOp_ = 0 + 1;
}

I ask because that's what I'd intended, but maybe clang-tidy thinks it's an erroneous use of std::forward.

I believe that the operation state type will be instantiated with either SomeSender or SomeSender& for Source, and then invoke the constructor with, correspondingly, either a SomeSender&& or a SomeSender&.

This diff is correct (except for a nit that I think the is_nothrow_connectable_v should reference Source2), but I think it communicates more generality than ever happens in practice; I'd expect static_assert(same_as<Source, Source2>) to always pass.

[ccotter]

Does clang-tidy complain about the following code?

To be up front, I implemented some of these checks in clang-tidy, including the cppcoreguidelines-rvalue-reference-param-not-moved check (which will be released in the next major version release). This code by default complains, but there is an option IgnoreNonDeducedTemplateTypes that can be set to true which silences this warning. Whether to warn on code such as this did come up in review, and it hinges on the meaning of the CppCoreGuidelines F.18 enforcement:

Flag all X&& parameters (where X is not a template type parameter name) where the function body uses them without std::move.

This is the discussion regarding this situation. Re-reading that convo and the CppCoreGuideline, it seems I interpreted the guideline to mean that non-deducing contexts are not considered in the where X is not a template type parameter name text. That doesn't quite seem right. Perhaps worth clarification from the guidelines authors, which I'll follow up on. At the very least, it seems that changing the default option IgnoreNonDeducedTemplateTypes to be true rather than defaulting false would make sense.

There is another related guideline ES.56 which has in its enforcement section

Flag when std::forward is applied to an rvalue reference (X&& where X is a non-template parameter type). Use std::move instead.
Flag when std::forward is applied to other than a forwarding reference. (More general case of the previous rule to cover the non-moving cases.)

I have always understood "a forwarding reference" to mean a reference deduced in a function call of the form T&& (which I think is confirmed in https://timsong-cpp.github.io/cppwp/n4861/temp.deduct.call#2.3.

template <class T>
struct AStruct {
    template <class U>
    void something(T&& t, U&& u) {} // T&& is not a forwarding reference, but U&& is
};

So, std::forward is not valid here (in the opinion of the CppCoreGuidelines, and a potential clang-tidy check I have ready to submit for review).

Summary

Does clang-tidy complain about the following code?

Yes, for the cppcoreguidelines-rvalue-reference-param-not-moved check, but it's configurable, and it also might be a bug in the tool not faithfully following the guideline, which I'll follow up on.

I ask because that's what I'd intended, but maybe clang-tidy thinks it's an erroneous use of std::forward.

Right, the code appears to not follow another CppCoreGuideline (for which there is no clang-tidy check yet, but hopefully will include on in the future) that forward should only be used on "forwarding references," and Source is not considered a forwarding reference in this code.

Next steps

Now, the CppCoreGuidelines are opinionated and intend to offer a set of "best practices" of sorts. Is it necessary to update let_done and others to have the operation constructor take a forwarding reference Source2&& and use forward, instead of how the code was originally written? I don't think so, and I think we can leave the code as is (if that's the preferred idiom you like, as it seems to be in working order in the let* algos) and still have the code pass clang-tidy (with appropriate options, and/or updating default option values in the tool itself). We'd have to use static_cast<Source&&>(source) instead of forward though, but that's "OK" in that the code in this case is using a non-CppCoreGuideline (and C++ standards I think) definition of "forwarding reference."

but I think it communicates more generality than ever happens in practice;

Agreed. That said, the generality can be reduced with constraints or static_asserts. That might not be a bad approach - using forward and the full generality, while explicitly spelling out the constraints to not allow misuse (of course, true C++20 constraints make this even more user friendly). To be honest, I did not realize that the operation accepted both lvaue refs and non-ref template types. I think it can be a bit more surprising to see a class template accepting both lvalue refs and non-ref template arguments without specialization for each (in this case, Source is used for computing result types but no object of type Source is a data member of the operation class)? That actually might just be my own assumptions though. Perhaps it's the notion of "forwarding reference for class templates" that is not as widespread of a practice compared to the well established forwarding references for function templates.

Let me know if I've understood things here. I appreciate you reviewing these (it's helped me "field test" these relatively new clang-tidy checks on real code, which is valuable feedback for the tool).

[ispeters]

I implemented some of these checks in clang-tidy,

Oh, cool!

At the very least, it seems that changing the default option IgnoreNonDeducedTemplateTypes to be true rather than defaulting false would make sense.

I wouldn't go that far. I think Unifex is pretty weird C++. The only other place I've seen a similar "saving for later" of a forwarding reference is a single example in some pretty obscure internal code.

I have always understood "a forwarding reference" to mean a reference deduced in a function call of the form T&&

Yeah, same.

Right, the code appears to not follow another CppCoreGuideline … that forward should only be used on "forwarding references," and Source is not considered a forwarding reference in this code.

Agreed, this isn't a traditional forwarding reference.

Regarding your next steps:

• I think we're on the same page that, given that the operation state class templates are sometimes instantiated with lvalue reference types for the source Sender, the templated constructor that takes a forwarding reference is obscuring what's really going on.
• I don't really like any of the options you've presented, but I think the least-bad is the static_cast.

Is there a way to suppress a clang-tidy warning for a given line of code, a bit like using #pragma to suppress compiler warnings? If there is, I think my preferred choice would be to use std::forward but suppress the "this isn't a forwarding reference" warning with an explanation.

As I alluded to above, I think of this case as a forwarding reference that's been "saved for later", almost like function currying, or a "lambda capture but for value category." As far as I know, this is an unusual pattern so I wouldn't go changing tool defaults based on it—I wouldn't even configure the tool for this library to silence the warning in this case. Absent an omniscient linter, I think this is a genuine case of not matching the recommended usage pattern and so a warning is appropriate.

I think this code deserves a comment; I had to reverse-engineer what it's doing to render you a decent code review. There's a fair amount of Unifex that's like that; we've inherited a prototype that I shipped to prod so I now have different optimization goals than the original authors did.

Summary

1. I wouldn't suggest you change your tool's defaults.
2. I would configure your tool to warn on this case for this library.
3. My preferred change is to use std::forward and disable the warning locally.
   • If local warning disablement is not possible, I'd use static_cast.
4. Regardless of the approach in 3, I think there should be a comment next to the cast that says something about how the not-really-a-forwarding-reference is effectively a forwarding reference that's been "saved for later" and now is later.

[ccotter]

Thanks for the detailed response.

We can go with std::forward with a local disablement. clang-tidy respects // NOLINT(<optional check name>) comments.

    return unifex::connect(
        std::forward<Source>(source),  // NOLINT(cppcoreguidelines-forward-non-forwarding-parameter)
        source_receiver{this});

// or, ignore all warnings

    return unifex::connect(std::forward<Source>(source), source_receiver{this}); // NOLINT

That said, cppcoreguidelines-forward-non-forwarding-parameter does not exist yet (hopefully that'll be an accepted check, possibly with a different name). As that check does not exist yet, I'll leave a comment, but that comment can later be changed (and/or kept) to a NOLINT for the specific warning.

[ispeters]

By a similar argument to the above, in let_done.hpp, I think that this expresses more generality than is possible given the change below, with the same curiosity about clang-tidy's attitude towards using forward with a class template parameter.

[ispeters]

Same as above in let_done and let_error. I think lines 378-379 are hand-rolling the member_t<Self, Predecessor> type alias with this:

decltype((static_cast<Sender&&>(sender).pred_)

[ccotter]

lines 378-379 are hand-rolling the member_t<Self, Predecessor> type alias with this:

Yep, I noticed this too, and added a test for "l value connectable"-ness though nothing had to be changed for this test to pass.

[ispeters]

This should probably be a move, too, which might mean it should be taken by rvalue reference.

Suggested change

	static_cast<SuccessorFactory&&>(func)(stopSource_),
	std::move(func)(stopSource_),

[ccotter]

SuccessorFactory&& also seems like a "saved for later forwarding reference," although it's used differently than in the other algos. Do we need func_ to remain alive for the duration of the operation? In the operation constructor, we currently have (annotated with // comment)

template <typename SuccessorFactory2, typename Receiver2>
  type(SuccessorFactory2&& func, Receiver2&& r) noexcept(
      std::is_nothrow_constructible_v<SuccessorFactory, SuccessorFactory2>&&
          std::is_nothrow_constructible_v<Receiver, Receiver2>&& noexcept(
              connect_inner_op(func, (Receiver2 &&) r))) // borrow func, whether it's an lvalue ref or rvalue erf
    : func_{(SuccessorFactory2 &&) func} // forward func into func_
    , receiverToken_(get_stop_token(r))
    , innerOp_(connect_inner_op(func_, (Receiver2 &&) r)) {} // borrow func_, but don't move so that func_ remains alive

SuccessorFactory& in connect_inner_op seems correct so that it borrows but doesn't possibly move from.