gemini-code-assist[bot] commented on code in PR #453: URL: https://github.com/apache/tvm-ffi/pull/453#discussion_r2810450038
########## include/tvm/ffi/extra/structural_key.h: ########## @@ -0,0 +1,90 @@ +/* + * 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; + explicit StructuralKeyObj(Any key) + : key(std::move(key)), hash_i64(static_cast<int64_t>(StructuralHash::Hash(this->key))) {} Review Comment:  The `StructuralKeyObj` constructor computes the structural hash using default arguments for `StructuralHash::Hash`, which means `skip_tensor_content=false`. However, both `StructuralKey::operator==` (line 72) and `ffi::StructuralKeyEqual` (in `src/ffi/extra/reflection_extra.cc`) use `StructuralEqual::Equal` with `skip_tensor_content=true`. This inconsistency violates the contract that if two objects are equal, their hashes must also be equal. For two `StructuralKey` objects wrapping tensors with the same shape but different content, the equality comparison would consider them equal (since `skip_tensor_content=true`), but their cached hashes would be different. This would lead to incorrect behavior in hash-based containers like `std::unordered_map` or `tvm::Map`. To fix this, the hash should be computed with the same `skip_tensor_content` policy as the equality check. ```suggestion : key(std::move(key)), hash_i64(static_cast<int64_t>(StructuralHash::Hash(this->key, /*map_free_vars=*/false, /*skip_tensor_content=*/true))) {} ``` -- 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]
