gemini-code-assist[bot] commented on code in PR #443:
URL: https://github.com/apache/tvm-ffi/pull/443#discussion_r2797530530
##########
python/tvm_ffi/container.py:
##########
@@ -124,6 +126,19 @@ def getitem_helper(
return elem_getter(obj, index)
+def normalize_index(length: int, idx: SupportsIndex) -> int:
+ """Normalize and bounds-check a Python index."""
+ try:
+ index = operator.index(idx)
+ except TypeError as exc: # pragma: no cover - defensive, matches list
behaviour
+ raise TypeError(f"indices must be integers or slices, not
{type(idx).__name__}") from exc
+ if index < -length or index >= length:
+ raise IndexError(f"Index out of range. size: {length}, got index
{index}")
+ if index < 0:
+ index += length
+ return index
Review Comment:
This new function `normalize_index` duplicates logic for index normalization
and bounds checking from the existing `getitem_helper` function. To improve
maintainability and avoid code duplication, consider refactoring
`getitem_helper` to use this new `normalize_index` function in a future change.
This would likely involve moving `normalize_index` before `getitem_helper` in
the file.
##########
include/tvm/ffi/container/list.h:
##########
@@ -0,0 +1,767 @@
+/*
+ * 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, protected TVMFFISeqCell {
+ public:
+ ~ListObj() {
+ Any* begin = MutableBegin();
+ for (int64_t i = 0; i < length; ++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>(length); }
+
+ /*!
+ * \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 >= length) {
+ TVM_FFI_THROW(IndexError) << "Index " << i << " out of bounds " <<
length;
+ }
+ return MutableBegin()[i];
+ }
+
+ /*! \return begin constant iterator */
+ const Any* begin() const { return MutableBegin(); }
+
+ /*! \return end constant iterator */
+ const Any* end() const { return MutableBegin() + length; }
+
+ /*! \brief Release reference to all the elements */
+ void clear() { ShrinkBy(length); }
+
+ /*!
+ * \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 >= length) {
+ TVM_FFI_THROW(IndexError) << "Index " << i << " out of bounds " <<
length;
+ }
+ 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->length = 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() + length; }
+
+ /*!
+ * \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 Assign elements into existing slots from [first, last).
+ * \param idx The starting point
+ * \param first Begin of iterator
+ * \param last End of iterator
+ * \tparam IterType The type of iterator
+ * \note Callers must ensure target slots are already initialized.
+ * \return Self
+ */
+ template <typename IterType>
+ ListObj* AssignRange(int64_t idx, IterType first, IterType last) {
+ Any* itr = MutableBegin() + idx;
+ for (; first != last; ++first) {
+ *itr++ = Any(*first);
+ }
+ return this;
+ }
+
+ /*!
+ * \brief Move elements from right to left, requires src_begin > dst
+ * \param dst Destination
+ * \param src_begin The start point of copy (inclusive)
+ * \param src_end The end point of copy (exclusive)
+ * \return Self
+ */
+ ListObj* MoveElementsLeft(int64_t dst, int64_t src_begin, int64_t src_end) {
+ Any* begin = MutableBegin();
+ std::move(begin + src_begin, begin + src_end, begin + dst);
+ return this;
+ }
+
+ /*!
+ * \brief Move elements from left to right, requires src_begin < dst
+ * \param dst Destination
+ * \param src_begin The start point of move (inclusive)
+ * \param src_end The end point of move (exclusive)
+ * \return Self
+ */
+ ListObj* MoveElementsRight(int64_t dst, int64_t src_begin, int64_t src_end) {
+ Any* begin = MutableBegin();
+ std::move_backward(begin + src_begin, begin + src_end, begin + dst +
(src_end - src_begin));
+ return this;
+ }
+
+ /*!
+ * \brief Enlarges the size of the list
+ * \param delta Size enlarged, should be positive
+ * \param val Default value
+ * \return Self
+ */
+ ListObj* EnlargeBy(int64_t delta, const Any& val = Any()) {
+ Any* itr = MutableEnd();
+ while (delta-- > 0) {
+ new (itr++) Any(val);
+ ++length;
+ }
+ return this;
+ }
+
+ /*!
+ * \brief Shrinks the size of the list
+ * \param delta Size shrinked, should be positive
+ * \return Self
+ */
+ ListObj* ShrinkBy(int64_t delta) {
+ Any* itr = MutableEnd();
+ while (delta-- > 0) {
+ (--itr)->Any::~Any();
+ --length;
+ }
+ return this;
+ }
+
+ /*!
+ * \brief Ensure the list has at least n slots.
+ * \param n The lower bound of required capacity.
+ */
+ void Reserve(int64_t n) {
+ if (n <= capacity) {
+ return;
+ }
+ Any* old_data = MutableBegin();
+ Any* new_data = static_cast<Any*>(::operator new(sizeof(Any) *
static_cast<size_t>(n)));
+ int64_t i = 0;
+ try {
+ for (; i < length; ++i) {
+ new (new_data + i) Any(std::move(old_data[i]));
+ }
+ } catch (...) {
+ for (int64_t j = 0; j < i; ++j) {
+ (new_data + j)->Any::~Any();
+ }
+ ::operator delete(new_data);
+ throw;
+ }
Review Comment:
The `try-catch` block around the element construction loop appears to be
unnecessary. The constructor being called here is `Any(Any&&)`, which is
declared `noexcept`. As such, the loop is not expected to throw an exception
that this block would catch. The main allocation that could throw (`::operator
new`) occurs before this block. Removing the `try-catch` would simplify the
code and make its intent clearer.
```c
for (; i < length; ++i) {
new (new_data + i) Any(std::move(old_data[i]));
}
```
--
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]
