junrushao commented on code in PR #78:
URL: https://github.com/apache/tvm-ffi/pull/78#discussion_r2404208762
##########
include/tvm/ffi/extra/module.h:
##########
@@ -201,7 +203,7 @@ class Module : public ObjectRef {
/*!
* \brief Property of ffi::Module
*/
- enum ModulePropertyMask : int {
+ enum ModulePropertyMask : std::uint8_t {
Review Comment:
Recommended by
[`performance-enum-size`](https://clang.llvm.org/extra/clang-tidy/checks/performance/enum-size.html)
but i guess it likely doesn't matter
##########
include/tvm/ffi/extra/module.h:
##########
@@ -233,7 +235,9 @@ class Module : public ObjectRef {
* \brief Constructor from ObjectPtr<ModuleObj>.
* \param ptr The object pointer.
*/
- explicit Module(ObjectPtr<ModuleObj> ptr) : ObjectRef(ptr) {
TVM_FFI_ICHECK(ptr != nullptr); }
+ explicit Module(const ObjectPtr<ModuleObj>& ptr) : ObjectRef(ptr) {
Review Comment:
Copy elision won't take place in the original impl
##########
include/tvm/ffi/container/variant.h:
##########
@@ -256,7 +257,7 @@ struct TypeTraits<Variant<V...>> : public TypeTraitsBase {
}
TVM_FFI_INLINE static Variant<V...> MoveFromAnyAfterCheck(TVMFFIAny* src) {
- return
Variant<V...>(details::AnyUnsafe::MoveTVMFFIAnyToAny(std::move(*src)));
+ return Variant<V...>(details::AnyUnsafe::MoveTVMFFIAnyToAny(src));
Review Comment:
`std::move` of the variable 'src' of the trivially-copyable type 'TVMFFIAny'
has no effect
##########
include/tvm/ffi/extra/module.h:
##########
@@ -113,7 +115,7 @@ class TVM_FFI_EXTRA_CXX_API ModuleObj : public Object {
* \param format Format of the source code, can be empty by default.
* \return Possible source code when available, or empty string if not
available.
*/
- virtual String InspectSource(const String& format = "") const { return
String(); }
+ virtual String InspectSource(const String& format) const { return String(); }
Review Comment:
Virtual method w/ default arguments is highly recommended against
##########
include/tvm/ffi/base_details.h:
##########
@@ -151,9 +151,9 @@
* This macro is used to clear the padding parts for hash and equality check
* in 32bit platform.
*/
-#define TVM_FFI_CLEAR_PTR_PADDING_IN_FFI_ANY(result) \
- if constexpr (sizeof((result)->v_obj) != sizeof((result)->v_int64)) { \
- (result)->v_int64 = 0; \
+#define TVM_FFI_CLEAR_PTR_PADDING_IN_FFI_ANY(result) \
+ if constexpr (sizeof(void*) != sizeof(int64_t)) { \
Review Comment:
Recommended by
[`bugprone-sizeof-expression`](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/sizeof-expression.html)
##########
include/tvm/ffi/c_api.h:
##########
@@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
-
+//
NOLINTBEGIN(performance-enum-size,modernize-use-using,bugprone-reserved-identifier,modernize-deprecated-headers)
Review Comment:
Skip diagnostics in C headers
##########
include/tvm/ffi/dtype.h:
##########
@@ -39,7 +40,7 @@ namespace ffi {
*
* This class is always consistent with the DLPack.
*/
-enum DLExtDataTypeCode { kDLExtCustomBegin = 129 };
+enum DLExtDataTypeCode : std::uint8_t { kDLExtCustomBegin = 129 };
Review Comment:
Ditto https://github.com/apache/tvm-ffi/pull/78/files#r2404208762
##########
src/ffi/extra/json_parser.cc:
##########
@@ -541,7 +542,8 @@ class JSONParser {
}
private:
- explicit JSONParser(String json_str) : ctx_(json_str.data(), json_str.data()
+ json_str.size()) {}
+ explicit JSONParser(String json_str)
+ : storage_(std::move(json_str)), ctx_(storage_.data(), storage_.data() +
storage_.size()) {}
Review Comment:
Added a field `storage_: String` to handle potential lifetime issue, but I
suppose in this specific case it could be false positive
##########
include/tvm/ffi/any.h:
##########
@@ -83,12 +83,12 @@ class AnyView {
/*! \brief Copy assignment operator */
AnyView& operator=(const AnyView&) = default;
/*! \brief Move constructor */
- AnyView(AnyView&& other) : data_(other.data_) {
+ AnyView(AnyView&& other) noexcept : data_(other.data_) {
Review Comment:
Recommended by
[`performance-noexcept-move-constructor`](https://clang.llvm.org/extra/clang-tidy/checks/performance/noexcept-move-constructor.html)
##########
include/tvm/ffi/reflection/access_path.h:
##########
@@ -40,7 +41,7 @@ namespace reflection {
/*!
* \brief The kind of the access pattern.
*/
-enum class AccessKind : int32_t {
+enum class AccessKind : std::uint8_t {
Review Comment:
Ditto
https://github.com/apache/tvm-ffi/pull/78/files#r2404208762
--
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]
