gemini-code-assist[bot] commented on code in PR #341:
URL: https://github.com/apache/tvm-ffi/pull/341#discussion_r2615544626
##########
src/ffi/container.cc:
##########
@@ -81,8 +86,17 @@ TVM_FFI_STATIC_INIT_BLOCK() {
[](const ffi::MapObj* n, const Any& k) -> int64_t {
return static_cast<int64_t>(n->count(k));
})
- .def("ffi.MapForwardIterFunctor", [](const ffi::MapObj* n) ->
ffi::Function {
- return ffi::Function::FromTyped(MapForwardIterFunctor(n->begin(),
n->end()));
+ .def("ffi.MapForwardIterFunctor",
+ [](const ffi::MapObj* n) -> ffi::Function {
+ return ffi::Function::FromTyped(MapForwardIterFunctor(n->begin(),
n->end()));
+ })
+ .def("ffi.MapGetMissingObject", GetMissingObject)
+ .def("ffi.MapGetItemOrMissing", [](const ffi::MapObj* n, const Any& k)
-> Any {
+ try {
+ return n->at(k);
+ } catch (const tvm::ffi::Error& e) {
+ return GetMissingObject();
+ }
Review Comment:
The current implementation of `MapGetItemOrMissing` is ambiguous. It returns
the sentinel object both when a key is not found, and when a key is found and
its associated value is the sentinel object itself. This makes it impossible
for the Python caller to distinguish between these two cases, leading to bugs
in `Map.get` and `ItemsView.__contains__`.
A more robust approach is to avoid a sentinel and return a tuple `(bool,
Any)` from the FFI function, where the boolean indicates if the key was found.
This is unambiguous and avoids the issues with sentinel values. This can be
implemented efficiently using `find` to avoid the `try-catch` overhead, which
also aligns with the performance goals of this PR.
```c
auto it = n->find(k);
if (it == n->end()) {
return std::make_tuple(false, Any(nullptr));
}
return std::make_tuple(true, it->second);
```
##########
python/tvm_ffi/container.py:
##########
@@ -349,10 +350,10 @@ def get(self, key: K, default: V | _DefaultT | None =
None) -> V | _DefaultT | N
The result value.
"""
- try:
- return self[key]
- except KeyError:
+ ret = _ffi_api.MapGetItemOrMissing(self, key)
+ if MISSING.same_as(ret):
return default
+ return ret
Review Comment:
The sentinel-based approach introduced here has a correctness issue. If the
`MISSING` object is stored as a value in the map, `Map.get` will incorrectly
return the `default` value. Similarly, `ItemsView.__contains__` (lines 259-261)
will fail to find items where the value is `MISSING`.
This is because the `MapGetItemOrMissing` FFI function is ambiguous: it
returns the same sentinel for both a missing key and a found key whose value is
the sentinel.
A more robust solution is to avoid a sentinel and have the FFI function
return a tuple `(bool, value)`, where the boolean indicates if the key was
found. I've left a suggestion on the C++ side to implement this.
If that change is made, this `get` method should be updated as follows:
```python
found, value = _ffi_api.MapGetItemOrMissing(self, key)
if not found:
return default
return value
```
And `ItemsView.__contains__` should be updated to:
```python
found, actual_value = _ffi_api.MapGetItemOrMissing(self._backend_map, key)
if not found:
return False
return actual_value == value
```
This would also mean `MapGetMissingObject` and the `MISSING` constant (line
80) are no longer needed and should be removed.
--
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]
