search

Kixiron / lasso

A fast, concurrent string interner
Apache License 2.0
137 stars 20 forks source link

Add blanket impls for `Resolver<K>`, `Reader<K>` and `Interner<K>` #31

Closed nerosnm closed 2 years ago

nerosnm commented 2 years ago

I recently came across a situation where I was storing a &mut Lasso and needed to pass it in to something that expected a T: Resolver, and found that &mut Lasso: !Resolver. I would assume this isn't a particularly controversial addition, since these blanket impls are a pretty standard thing, but let me know if I've missed something important here.

Kixiron commented 2 years ago

It looks like the blanket implementations conflict with the manual implementations for &ThreadedRodeo, could you remove those?

nerosnm commented 2 years ago

Whoops, I forgot that was feature-flagged. I've removed the conflicting impl.

I also see that there are &ThreadedRodeo: Interner and &ThreadedRodeo: Reader impls, so I can add the same blanket impls for those two traits.

nerosnm commented 2 years ago

I wasn't able to implement Interner for &T, only &mut T, because of the &mut self in the signature of the Interner methods. If I'm honest, I'm not 100% sure why the existing impl for &ThreadedRodeo<K, S> works (in src/interface/threaded_ref.rs).

Maybe I'm missing something? Oh, it's because that impl is calling the inherent methods on ThreadedRodeo that only take &self. So that will have to stay as a concrete impl. I guess that's fine, because I wouldn't expect a trait like Interner that takes &mut self to be available through &T in general anyway.

Kixiron commented 2 years ago

Looks great, thank you!