New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature Request] Use Tuple[K, V] instead of DictEntry[K, V] #2554
Comments
I would like to work on this feature. |
@gabrieldemarmiesse If we replace with Tuple[K, V], how do we deal with the key hash? @value
struct DictEntry[K: KeyElement, V: CollectionElement](CollectionElement):
"""Store a key-value pair entry inside a dictionary.
Parameters:
K: The key type of the dict. Must be Hashable+EqualityComparable.
V: The value type of the dict.
"""
var hash: Int
"""`key.__hash__()`, stored so hashing isn't re-computed during dict lookup."""
var key: K
"""The unique key for the entry."""
var value: V
"""The value associated with the key."""
fn __init__(inout self, owned key: K, owned value: V):
"""Create an entry from a key and value, computing the hash.
Args:
key: The key of the entry.
value: The value of the entry.
"""
self.hash = hash(key)
self.key = key^
self.value = value^ |
We can make DictEntry an internal structure and only return Tuple[K, V] to the user. |
@jayzhan211 are you working on this issue? If yes, please, continue. If not, I will start working on it. |
not working on it. |
I started working on it. A few questions:
Thank you. |
Thanks for working on this!
The Modular staff is currently working on a benchmarking infrastructure. It's not ready yet. But in general avoiding copies is always a good way to have good performance out of the box. |
I have a very unusual issue. I tried Tuple[K, V], and the tests pass, but when I try Tuple[Reference[K,...], Reference[V, ...]], it throws a weird error during the test run (nothing when building the library). The error is here:
And here is the code - https://github.com/VMois/mojo/pull/1/files (previous commits have Tuple[K, V] changes). The only difference between the current and expected types is Could someone help? Maybe my approach is wrong. Thank you. |
Should we replace DictEntry with Tuple in Upd: I rethink about the issue here. Given that we cant remove DictEntry because we need keyhash, I think replacing DictEntry with Tuple has no benefit at all. |
Review Mojo's priorities
What is your request?
As title
What is your motivation for this change?
DictEntry was there because Tuple only worked with AnyRegType, now that
Tuple
works withAnyType
, there is no need forDictEntry
anymore. We can just return Tuple when working withDict.items()
Reference:
Any other details?
No response
The text was updated successfully, but these errors were encountered: