gemini-code-assist[bot] commented on code in PR #443:
URL: https://github.com/apache/tvm-ffi/pull/443#discussion_r2796313584
##########
src/ffi/extra/structural_hash.cc:
##########
@@ -193,6 +197,15 @@ class StructuralHashHandler {
return hash_value;
}
+ // NOLINTNEXTLINE(performance-unnecessary-value-param)
+ uint64_t HashList(List<Any> list) {
+ uint64_t hash_value = details::StableHashCombine(list->GetTypeKeyHash(),
list.size());
+ for (const auto& elem : list) {
+ hash_value = details::StableHashCombine(hash_value, HashAny(elem));
+ }
+ return hash_value;
+ }
Review Comment:
The function `HashList` is a verbatim copy of `HashArray`. To improve
maintainability and avoid code duplication, consider refactoring this into a
template function that can operate on any container that is iterable and has
`size()` and `GetTypeKeyHash()` methods.
For example:
```cpp
template <typename TContainer>
uint64_t HashSequence(TContainer container) {
uint64_t hash_value =
details::StableHashCombine(container->GetTypeKeyHash(), container.size());
for (const auto& elem : container) {
hash_value = details::StableHashCombine(hash_value, HashAny(elem));
}
return hash_value;
}
// Then in HashArray and HashList:
uint64_t HashArray(Array<Any> arr) {
return HashSequence(arr);
}
uint64_t HashList(List<Any> list) {
return HashSequence(list);
}
```
This would make the code more generic and easier to maintain.
##########
include/tvm/ffi/container/list.h:
##########
@@ -0,0 +1,784 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file tvm/ffi/container/list.h
+ * \brief Mutable list type.
+ *
+ * tvm::ffi::List<Any> is an erased mutable sequence container.
+ */
+#ifndef TVM_FFI_CONTAINER_LIST_H_
+#define TVM_FFI_CONTAINER_LIST_H_
+
+#include <tvm/ffi/any.h>
+#include <tvm/ffi/container/array.h>
+#include <tvm/ffi/object.h>
+
+#include <algorithm>
+#include <initializer_list>
+#include <new>
+#include <sstream>
+#include <string>
+#include <type_traits>
+#include <utility>
+#include <vector>
+
+namespace tvm {
+namespace ffi {
+
+/*! \brief List node content in list */
+class ListObj : public Object {
+ public:
+ ~ListObj() {
+ Any* begin = MutableBegin();
+ for (int64_t i = 0; i < size_; ++i) {
+ (begin + i)->Any::~Any();
+ }
+ TVM_FFI_ICHECK(data_deleter_ != nullptr);
+ data_deleter_(data_);
+ }
+
+ /*! \return The size of the list */
+ size_t size() const { return static_cast<size_t>(size_); }
+
+ /*!
+ * \brief Read i-th element from list.
+ * \param i The index
+ * \return the i-th element.
+ */
+ const Any& at(int64_t i) const { return this->operator[](i); }
+
+ /*!
+ * \brief Read i-th element from list.
+ * \param i The index
+ * \return the i-th element.
+ */
+ const Any& operator[](int64_t i) const {
+ if (i < 0 || i >= size_) {
+ TVM_FFI_THROW(IndexError) << "Index " << i << " out of bounds " << size_;
+ }
+ return MutableBegin()[i];
+ }
+
+ /*! \return begin constant iterator */
+ const Any* begin() const { return MutableBegin(); }
+
+ /*! \return end constant iterator */
+ const Any* end() const { return MutableBegin() + size_; }
+
+ /*! \brief Release reference to all the elements */
+ void clear() { ShrinkBy(size_); }
+
+ /*!
+ * \brief Set i-th element of the list in-place
+ * \param i The index
+ * \param item The value to be set
+ */
+ void SetItem(int64_t i, Any item) {
+ if (i < 0 || i >= size_) {
+ TVM_FFI_THROW(IndexError) << "Index " << i << " out of bounds " << size_;
+ }
+ MutableBegin()[i] = std::move(item);
+ }
+
+ /*!
+ * \brief Constructs a container with n elements. Each element is a copy of
val
+ * \param n The size of the container
+ * \param val The init value
+ * \return Ref-counted ListObj requested
+ */
+ static ObjectPtr<ListObj> CreateRepeated(int64_t n, const Any& val) {
+ ObjectPtr<ListObj> p = ListObj::Empty(n);
+ Any* itr = p->MutableBegin();
+ for (int64_t& i = p->size_ = 0; i < n; ++i) {
+ new (itr++) Any(val);
+ }
+ return p;
+ }
+
+ /// \cond Doxygen_Suppress
+ static constexpr const int32_t _type_index = TypeIndex::kTVMFFIList;
+ static const constexpr bool _type_final = true;
+ TVM_FFI_DECLARE_OBJECT_INFO_STATIC(StaticTypeKey::kTVMFFIList, ListObj,
Object);
+ /// \endcond
+
+ private:
+ /*! \return begin mutable iterator */
+ Any* MutableBegin() const { return static_cast<Any*>(this->data_); }
+
+ /*! \return end mutable iterator */
+ Any* MutableEnd() const { return MutableBegin() + size_; }
+
+ /*!
+ * \brief Emplace a new element at the given index
+ * \param idx The index of the element.
+ * \param args The arguments to construct the new element
+ */
+ template <typename... Args>
+ void EmplaceInit(size_t idx, Args&&... args) {
+ Any* itr = MutableBegin() + idx;
+ new (itr) Any(std::forward<Args>(args)...);
+ }
+
+ /*!
+ * \brief Inplace-initialize the elements starting idx from [first, last)
+ * \param idx The starting point
+ * \param first Begin of iterator
+ * \param last End of iterator
+ * \tparam IterType The type of iterator
+ * \return Self
+ */
+ template <typename IterType>
+ ListObj* InitRange(int64_t idx, IterType first, IterType last) {
+ Any* itr = MutableBegin() + idx;
+ for (; first != last; ++first) {
+ Any ref = *first;
+ new (itr++) Any(std::move(ref));
+ }
Review Comment:
The implementation `Any ref = *first; new (itr++) Any(std::move(ref));`
involves an unnecessary copy and move. This can be simplified to a direct copy
construction, which is more concise and avoids the temporary variable.
```c
for (; first != last; ++first) {
new (itr++) Any(*first);
}
```
##########
include/tvm/ffi/container/list.h:
##########
@@ -0,0 +1,784 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*!
+ * \file tvm/ffi/container/list.h
+ * \brief Mutable list type.
+ *
+ * tvm::ffi::List<Any> is an erased mutable sequence container.
+ */
+#ifndef TVM_FFI_CONTAINER_LIST_H_
+#define TVM_FFI_CONTAINER_LIST_H_
+
+#include <tvm/ffi/any.h>
+#include <tvm/ffi/container/array.h>
+#include <tvm/ffi/object.h>
+
+#include <algorithm>
+#include <initializer_list>
+#include <new>
+#include <sstream>
+#include <string>
+#include <type_traits>
+#include <utility>
+#include <vector>
+
+namespace tvm {
+namespace ffi {
+
+/*! \brief List node content in list */
+class ListObj : public Object {
+ public:
+ ~ListObj() {
+ Any* begin = MutableBegin();
+ for (int64_t i = 0; i < size_; ++i) {
+ (begin + i)->Any::~Any();
+ }
+ TVM_FFI_ICHECK(data_deleter_ != nullptr);
+ data_deleter_(data_);
+ }
+
+ /*! \return The size of the list */
+ size_t size() const { return static_cast<size_t>(size_); }
+
+ /*!
+ * \brief Read i-th element from list.
+ * \param i The index
+ * \return the i-th element.
+ */
+ const Any& at(int64_t i) const { return this->operator[](i); }
+
+ /*!
+ * \brief Read i-th element from list.
+ * \param i The index
+ * \return the i-th element.
+ */
+ const Any& operator[](int64_t i) const {
+ if (i < 0 || i >= size_) {
+ TVM_FFI_THROW(IndexError) << "Index " << i << " out of bounds " << size_;
+ }
+ return MutableBegin()[i];
+ }
+
+ /*! \return begin constant iterator */
+ const Any* begin() const { return MutableBegin(); }
+
+ /*! \return end constant iterator */
+ const Any* end() const { return MutableBegin() + size_; }
+
+ /*! \brief Release reference to all the elements */
+ void clear() { ShrinkBy(size_); }
+
+ /*!
+ * \brief Set i-th element of the list in-place
+ * \param i The index
+ * \param item The value to be set
+ */
+ void SetItem(int64_t i, Any item) {
+ if (i < 0 || i >= size_) {
+ TVM_FFI_THROW(IndexError) << "Index " << i << " out of bounds " << size_;
+ }
+ MutableBegin()[i] = std::move(item);
+ }
+
+ /*!
+ * \brief Constructs a container with n elements. Each element is a copy of
val
+ * \param n The size of the container
+ * \param val The init value
+ * \return Ref-counted ListObj requested
+ */
+ static ObjectPtr<ListObj> CreateRepeated(int64_t n, const Any& val) {
+ ObjectPtr<ListObj> p = ListObj::Empty(n);
+ Any* itr = p->MutableBegin();
+ for (int64_t& i = p->size_ = 0; i < n; ++i) {
+ new (itr++) Any(val);
+ }
Review Comment:
The for-loop idiom used here to initialize the list and increment its size
is a bit dense and could be difficult for other developers to understand at a
glance. A more conventional loop that explicitly increments the size would be
more readable and just as exception-safe. This also applies to the `Assign`
method later in this file.
For example:
```cpp
p->size_ = 0;
for (int64_t i = 0; i < n; ++i) {
new (itr++) Any(val);
p->size_++;
}
```
##########
src/ffi/extra/structural_equal.cc:
##########
@@ -333,6 +338,39 @@ class StructEqualHandler {
return false;
}
+ // NOLINTNEXTLINE(performance-unnecessary-value-param)
+ bool CompareList(ffi::List<Any> lhs, ffi::List<Any> rhs) {
+ if (lhs.size() != rhs.size()) {
+ // fast path, size mismatch, and there is no path tracing
+ // return false since we don't need informative error message
+ if (mismatch_lhs_reverse_path_ == nullptr) return false;
+ }
+ for (int64_t i = 0, n = static_cast<int64_t>(std::min(lhs.size(),
rhs.size())); i < n; ++i) {
+ if (!CompareAny(lhs[i], rhs[i])) {
+ if (mismatch_lhs_reverse_path_ != nullptr) {
+
mismatch_lhs_reverse_path_->emplace_back(reflection::AccessStep::ArrayItem(i));
+
mismatch_rhs_reverse_path_->emplace_back(reflection::AccessStep::ArrayItem(i));
+ }
+ return false;
+ }
+ }
+ if (lhs.size() == rhs.size()) return true;
+ if (mismatch_lhs_reverse_path_ != nullptr) {
+ if (lhs.size() > rhs.size()) {
+ mismatch_lhs_reverse_path_->emplace_back(
+
reflection::AccessStep::ArrayItem(static_cast<int64_t>(rhs.size())));
+ mismatch_rhs_reverse_path_->emplace_back(
+
reflection::AccessStep::ArrayItemMissing(static_cast<int64_t>(rhs.size())));
+ } else {
+ mismatch_lhs_reverse_path_->emplace_back(
+
reflection::AccessStep::ArrayItemMissing(static_cast<int64_t>(lhs.size())));
+ mismatch_rhs_reverse_path_->emplace_back(
+
reflection::AccessStep::ArrayItem(static_cast<int64_t>(lhs.size())));
+ }
+ }
+ return false;
+ }
Review Comment:
The function `CompareList` is a verbatim copy of `CompareArray`. To improve
maintainability and avoid code duplication, consider refactoring this into a
template function that can operate on any container with `size()` and
`operator[]` methods, like `Array` and `List`.
For example:
```cpp
template <typename TContainer>
bool CompareSequence(TContainer lhs, TContainer rhs) {
if (lhs.size() != rhs.size()) {
if (mismatch_lhs_reverse_path_ == nullptr) return false;
}
// ... implementation ...
}
// Then in CompareArray and CompareList:
bool CompareArray(ffi::Array<Any> lhs, ffi::Array<Any> rhs) {
return CompareSequence(lhs, rhs);
}
bool CompareList(ffi::List<Any> lhs, ffi::List<Any> rhs) {
return CompareSequence(lhs, rhs);
}
```
This would make the code more generic and easier to maintain.
--
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]
