gemini-code-assist[bot] commented on code in PR #453: URL: https://github.com/apache/tvm-ffi/pull/453#discussion_r2810484670
########## include/tvm/ffi/extra/structural_key.h: ########## @@ -0,0 +1,102 @@ +/* + * 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/extra/structural_key.h + * \brief Structural-key wrapper that caches structural hash. + */ +#ifndef TVM_FFI_EXTRA_STRUCTURAL_KEY_H_ +#define TVM_FFI_EXTRA_STRUCTURAL_KEY_H_ + +#include <tvm/ffi/any.h> +#include <tvm/ffi/extra/structural_equal.h> +#include <tvm/ffi/extra/structural_hash.h> + +#include <cstddef> +#include <cstdint> +#include <functional> +#include <utility> + +namespace tvm { +namespace ffi { + +/*! + * \brief Object node that contains a key and its cached structural hash. + */ +class StructuralKeyObj : public Object { + public: + /*! \brief The key value. */ + Any key; + /*! \brief Cached structural hash of \p key, encoded as int64_t. */ + int64_t hash_i64; + + // Default constructor to support reflection-based initialization. + StructuralKeyObj() = default; + /*! + * \brief Construct a StructuralKeyObj from a key and cache its structural hash. + * \param key The key value. + */ + explicit StructuralKeyObj(Any key) + : key(std::move(key)), hash_i64(static_cast<int64_t>(StructuralHash::Hash(this->key))) {} + + /// \cond Doxygen_Suppress + TVM_FFI_DECLARE_OBJECT_INFO_FINAL("ffi.StructuralKey", StructuralKeyObj, Object); + /// \endcond +}; + +/*! + * \brief ObjectRef wrapper for StructuralKeyObj. + * + * StructuralKey can be used to ensure that we use structural equality and hash for the wrapped key. + */ +class StructuralKey : public ObjectRef { + public: + /*! + * \brief Construct a StructuralKey from a key. + * \param key The key value. + */ + explicit StructuralKey(Any key) : ObjectRef(make_object<StructuralKeyObj>(std::move(key))) {} + + bool operator==(const StructuralKey& other) const { + if (this->same_as(other)) { + return true; + } + if (this->get()->hash_i64 != other->hash_i64) { + return false; + } + return StructuralEqual::Equal(this->get()->key, other->key); + } Review Comment:  Since `StructuralKey` is an `ObjectRef`, it inherits `operator!=` from the base class, which performs pointer inequality. By overriding `operator==` to use structural equality without also overriding `operator!=`, you create an inconsistency where `k1 == k2` might be true (structurally) but `k1 != k2` could also be true (different pointers). You should explicitly define `operator!=` to maintain consistency. ```suggestion bool operator==(const StructuralKey& other) const { if (this->same_as(other)) { return true; } if (this->get()->hash_i64 != other->hash_i64) { return false; } return StructuralEqual::Equal(this->get()->key, other->key); } bool operator!=(const StructuralKey& other) const { return !(*this == other); } ``` ########## include/tvm/ffi/extra/structural_key.h: ########## @@ -0,0 +1,102 @@ +/* + * 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/extra/structural_key.h + * \brief Structural-key wrapper that caches structural hash. + */ +#ifndef TVM_FFI_EXTRA_STRUCTURAL_KEY_H_ +#define TVM_FFI_EXTRA_STRUCTURAL_KEY_H_ + +#include <tvm/ffi/any.h> +#include <tvm/ffi/extra/structural_equal.h> +#include <tvm/ffi/extra/structural_hash.h> + +#include <cstddef> +#include <cstdint> +#include <functional> +#include <utility> + +namespace tvm { +namespace ffi { + +/*! + * \brief Object node that contains a key and its cached structural hash. + */ +class StructuralKeyObj : public Object { + public: + /*! \brief The key value. */ + Any key; + /*! \brief Cached structural hash of \p key, encoded as int64_t. */ + int64_t hash_i64; Review Comment:  The `hash_i64` member is not initialized in the default constructor. While reflection-based initialization usually overwrites it, it is safer to provide a default value to avoid indeterminate state if the object is accessed before being fully initialized. ```suggestion int64_t hash_i64{0}; ``` -- 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]
