michaelsproul / rust_radix_trie

Fast generic radix trie implemented in Rust
https://docs.rs/radix_trie/
MIT License
184 stars 32 forks source link

Question about SubTrie methods consuming self instead of taking it by reference #74

Closed vkurilin closed 2 months ago

vkurilin commented 2 months ago

Hello!

Is there a specific reason why most access methods in SubTrie consume self rather than taking it by a constant reference?

For example, it seems that key() and value() from TrieCommon could take &self, which would enable code like this:

if let Some(ancestor) = self.trie.get_ancestor(key) {
    if ancestor.key() == /* some logic here */ {
        return ancestor.value();
    }
}

This change would allow us to avoid the need to create temporary variables or clones, potentially improving both performance and readability. Could you please provide some insight into this design choice?

Thank you!

michaelsproul commented 2 months ago

It's so TrieCommon can be implemented on & and &mut types. I forget the details, but this was to make lifetimes play nicely.

You shouldn't need to clone anything to call .key() on SubTrie, because the TrieCommon trait is implemented on &SubTrie:

https://docs.rs/radix_trie/latest/radix_trie/trait.TrieCommon.html#implementors

michaelsproul commented 2 months ago

I tried adding your code example to examples/debug.rs and it works without issue:

    if let Some(ancestor) = trie.get_ancestor("hello world") {
        if ancestor.key() == Some(&"hello") {
            println!("{:?}", ancestor.value());
        }
    }

Can you elaborate on the error you hit?

vkurilin commented 2 months ago

Hmm, looks like I jumped to conclusions.

In a large project with a complex type, I encountered the error "rust value used after move," on this line, saw self in the trait, and thought — here it is 🙈

In a clean example, it indeed does not reproduce.

I tested it with primitive types and defined my own non-copy type with custom Eq.

Everything works as expected; this issue can be closed.

Thank you so much for such a quick response!