Add static_assert to prevent implicit conversion of sequencer return type

[dbiswas2808]

Currently the sequencer node may implicitly convert a signed sequence key to size_t type. To add some safety from the wraparound issue with unsigned integer I propose that the caller should be forced to define a sequencer that explicitly returns a size_t type. For example, I have seen the following issue happen in practice where the sequencer lambda returns a signed integer and when ii < 0 the value get's wrapped around.

[](int ii) {
   return ii;
}

I was willing to put this check in function_body_leaf, but their maybe some use cases where we would want to preserve the implicit conversion behavior, not sure.

[kboyarinov]

@dbiswas2808, thank you for reporting this and making the PR. The proposed change can definitely prevent issues like you described but in the same time - it can break some pre-existing code that worked fine with the conversion. Currently, starting from C++20 oneapi::tbb::sequencer_node is constrained and by default, only the instantiation with the body that returns std::size_t would be well-formed. If there is some pre-existing code that is not well-formed with this constraint, the user can specify the special macro that allows the return type of the lambda to be convertible to std::size_t. Such an approach can be also done with static_assert until C++20. @aleksei-fedotov, what do you think?

[dbiswas2808]

@dbiswas2808, thank you for reporting this and making the PR. The proposed change can definitely prevent issues like you described but in the same time - it can break some pre-existing code that worked fine with the conversion. Currently, starting from C++20 oneapi::tbb::sequencer_node is constrained and by default, only the instantiation with the body that returns std::size_t would be well-formed. If there is some pre-existing code that is not well-formed with this constraint, the user can specify the special macro that allows the return type of the lambda to be convertible to std::size_t. Such an approach can be also done with static_assert until C++20. @aleksei-fedotov, what do you think?

@kboyarinov that is a good point. I think adding a macro to enable this is a good step. Other places I can think of extending this check is for tag matching specialization of join_node. There maybe others

[kboyarinov]

@dbiswas2808, as for me, the proposed change is a part of the more wide question - how oneTBB library checks the user-defined type to be compliant with the corresponding named requirements. It affects almost all oneTBB interfaces and if some changes are required - it is required for all of them. It also may result in some unexpected user code that works previously to stop working. And taking into account the fact that this work is done already starting from C++20 because of constraints, it is recommended to switch into C++20 to enable this feature and not to add static assertions into all oneTBB interfaces.

[dbiswas2808]

@dbiswas2808, as for me, the proposed change is a part of the more wide question - how oneTBB library checks the user-defined type to be compliant with the corresponding named requirements. It affects almost all oneTBB interfaces and if some changes are required - it is required for all of them. It also may result in some unexpected user code that works previously to stop working. And taking into account the fact that this work is done already starting from C++20 because of constraints, it is recommended to switch into C++20 to enable this feature and not to add static assertions into all oneTBB interfaces.

I agree. I think C++20 provides most of the tools to handle this sort of thing in a more elegant way. Thanks for the suggestion and your feedback.