gemini-code-assist[bot] commented on code in PR #229:
URL: https://github.com/apache/tvm-ffi/pull/229#discussion_r2495926452
##########
include/tvm/ffi/function_details.h:
##########
@@ -137,8 +139,17 @@ using FGetFuncSignature = std::string (*)();
/*!
* \brief Auxilary argument value with context for error reporting
*/
-class ArgValueWithContext {
+template <typename Type, bool NeedStorage>
+class ArgValueWithContext;
+
+template <typename Type>
+class ArgValueWithContext<Type, /*NeedStorage=*/false> {
public:
+ // We only support pure value type to convert to, to avoid any dangling
reference issues.
+ // C++ will automatically eliminate unnecessary constructor calls by NRVO,
+ // so return by value won't cause performance issue.
+ static_assert(std::is_same_v<Type,
std::remove_const_t<std::remove_reference_t<Type>>>);
+
/*!
* \brief move constructor from another return value.
Review Comment:
The Doxygen `\brief` for this constructor is misleading. It states this is a
"move constructor", but it's a regular constructor. This should be corrected to
avoid confusion.
```c
* \brief Construct a new ArgValueWithContext object.
```
##########
include/tvm/ffi/function_details.h:
##########
@@ -182,11 +189,75 @@ class ArgValueWithContext {
FGetFuncSignature f_sig_;
};
+template <typename Type>
+class ArgValueWithContext<Type, /*NeedStorage=*/true> {
+ public:
+ // We only support pure value type to convert to, to avoid any dangling
reference issues.
+ // C++ will automatically eliminate unnecessary constructor calls by NRVO,
+ // so return by value won't cause performance issue.
+ static_assert(std::is_same_v<Type,
std::remove_const_t<std::remove_reference_t<Type>>>);
+ /*!
+ * \brief move constructor from another return value.
Review Comment:
The Doxygen `\brief` for this constructor is misleading. It states this is a
"move constructor", but it's a regular constructor. This should be corrected to
avoid confusion.
```c
* \brief Construct a new ArgValueWithContext object.
```
##########
include/tvm/ffi/function_details.h:
##########
@@ -182,11 +189,75 @@ class ArgValueWithContext {
FGetFuncSignature f_sig_;
};
+template <typename Type>
+class ArgValueWithContext<Type, /*NeedStorage=*/true> {
+ public:
+ // We only support pure value type to convert to, to avoid any dangling
reference issues.
+ // C++ will automatically eliminate unnecessary constructor calls by NRVO,
+ // so return by value won't cause performance issue.
+ static_assert(std::is_same_v<Type,
std::remove_const_t<std::remove_reference_t<Type>>>);
+ /*!
+ * \brief move constructor from another return value.
+ * \param args The argument list
+ * \param arg_index In a function call, this argument is at index arg_index
(0-indexed).
+ * \param optional_name Name of the function being called. Can be nullptr if
the function is
+ * not.
+ * \param f_sig Pointer to static function outputting signature of the
function being called.
+ * named.
Review Comment:
The Doxygen comments for `optional_name` and `f_sig` are malformed. The
descriptions are split incorrectly across multiple lines, which makes the
documentation hard to read. Let's fix the formatting.
```c
* \param optional_name Name of the function being called. Can be nullptr
if the function is not named.
* \param f_sig Pointer to static function outputting signature of the
function being called.
```
--
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]
