gemini-code-assist[bot] commented on code in PR #266:
URL: https://github.com/apache/tvm-ffi/pull/266#discussion_r2529981980
##########
include/tvm/ffi/function.h:
##########
@@ -305,27 +314,28 @@ class Function : public ObjectRef {
* \param packed_call The packed function signature
* \note legacy purpose, should change to Function::FromPacked for
mostfuture use.
*/
- template <typename TCallable>
- explicit Function(TCallable packed_call) {
- *this = FromPacked(packed_call);
+ template <typename TCallable,
+ typename =
std::enable_if_t<!std::is_same_v<std::decay_t<TCallable>, Function>>>
+ explicit Function(TCallable&& packed_call) {
+ *this = FromPacked(std::forward<TCallable>(packed_call));
}
/*!
* \brief Constructing a packed function from a callable type
* whose signature is consistent with `ffi::Function`
* \param packed_call The packed function signature
*/
- template <typename TCallable> // //
NOLINTNEXTLINE(performance-unnecessary-value-param)
- static Function FromPacked(TCallable packed_call) {
+ template <typename TCallable>
+ static Function FromPacked(TCallable&& packed_call) {
static_assert(
std::is_convertible_v<TCallable, std::function<void(const AnyView*,
int32_t, Any*)>> ||
std::is_convertible_v<TCallable, std::function<void(PackedArgs
args, Any*)>>,
"tvm::ffi::Function::FromPacked requires input function signature to
match packed func "
"format");
if constexpr (std::is_convertible_v<TCallable,
std::function<void(PackedArgs args, Any*)>>) {
return FromPackedInternal(
- [packed_call](const AnyView* args, int32_t num_args, Any* rv)
mutable -> void {
- PackedArgs args_pack(args, num_args);
- packed_call(args_pack, rv);
+ [packed_call = std::forward<TCallable>(packed_call)](
+ const AnyView* args, int32_t num_args, Any* rv) mutable -> void {
+ packed_call(PackedArgs{args, num_args}, rv);
});
} else {
return FromPackedInternal(packed_call);
Review Comment:
The perfect forwarding is broken in this code path. Since `packed_call` is a
function parameter, it is an lvalue within this function's scope. Calling
`FromPackedInternal(packed_call)` will therefore always pass it as an lvalue,
causing it to be copied instead of moved, even if an rvalue was originally
passed to `FromPacked`. To preserve the value category of the original
argument, you should use `std::forward`.
```c
return FromPackedInternal(std::forward<TCallable>(packed_call));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
