search

tov / succinct-rs

Succinct Data Structures for Rust
Apache License 2.0
56 stars 15 forks source link

Make the backing store for JacobsonRank COW. #1

Closed danieldk closed 5 years ago

danieldk commented 8 years ago

I wanted store the JacobsonRank struct in another struct. However, this gives typical ownership problems (since the ownership is tied to a scope and it's not possible to move the backing store and construct the rank at the same time in struct initialization).

This is a suggested change that makes uses the Cow type to offer the option of letting JacobsonRank own the store as well.

I don't think this is as much of a problem for BinSearchSelect, since its construction is trivial and one can always make instances on the fly.

tov commented 5 years ago

Hey! It looks like this has been sitting here for more than two years, and I didn't see it until just now. Sorry! I think it's probably a good idea, though I need to look at it a bit more carefully, and it looks like it now conflicts with some other change.

I don't have a lot of time to work on this crate right now. Are you (still) using it for something?

danieldk commented 5 years ago

No problem! I am not using the crate at this moment, but I can redo this PR once I'd need this functionality again.

danieldk commented 5 years ago

If you don't mind, I'll close this PR, so that it's off my list of open PRs ;).

tov commented 5 years ago

I don’t mind, though I do kind of want it, and you should get credit if/when I merge it. When I designed the JacobsonRank struct initially, I wasn't sure whether it should own or borrow the backing store. Both ways seemed potentially useful, and it never even occurred to me to use a Cow! But now that you’ve done it (over two years ago), it seems like it solves the problem.

Now I'm wondering, though: Is there ever a reason that we’d use Cow::to_mut on it? If if not, do we actually need a Cow and ToOwned? I guess we do want something that impls From or Into in the right way, and a Cow doesn't cost anything over a bespoke enum, right?

Is there a reason it shouldn't implement Debug?