Skip to content
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

Documentation introduction could be improved #8

Closed
mcclure opened this issue Dec 15, 2022 · 3 comments
Closed

Documentation introduction could be improved #8

mcclure opened this issue Dec 15, 2022 · 3 comments

Comments

@mcclure
Copy link

mcclure commented Dec 15, 2022

I used RangeSet for a project today. Although it was helpful, I was initially very confused how to use it. Some very important information about RangeSet is actually not in the docs.rs docs but rather is in either the github README or in comments to issues.

I think the following things should be made clear at either https://docs.rs/range-set or https://docs.rs/range-set/0.0.9/range_set/struct.RangeSet.html. If you like, I can submit a PR adding these things to the documentation, but first I would need you to confirm my current understanding of them is correct.

  1. What does the type mean?

    The example type of a RangeSet given in the docs is RangeSet<[RangeInclusive <u32>; 1]> I could understand RangeSet<u32> or RangeSet<RangeInclusive<u32>>, but what is the purpose of the array?

    I think?: From reading the README I think the idea here is that for a ; N] array, as an optimization, if the set consists of no more than N elements the elements will be stored entirely within the object ("on the stack") with no additional allocation. Over N it still works, but starts using the heap.

    Still don't get: Is it legal to use other range types, such as `RangeSet<[Range ]; 1]>?

  2. How do you iterate over ranges, rather than values?

    I think?: Per this issue the answer appears to be to say s.as_ref().iter() instead of s.iter().

  3. What are the purpose of spilled() and shrink_to_fit()?

    I think?: These are again about the "on stack" optimization, and they are explained in the smallvec documentation.

    Still don't get: Under what circumstances would shrink_to_fit() become relevant? I guess if you add a bunch of ranges forcing a spill and then remove some, the ranges might still be unnecessarily spilled until you shrink()?

  4. Is it possible to immutably query the intersection of two range_sets?

    I think?: You can get the intersection using insert_range() or remove_range(), but there is no way to do this without modifying the range_set.

Thanks.

@spearman
Copy link
Owner

spearman commented Dec 19, 2022

Thanks for the feedback.

  1. Yes that's right. In 0b88769 and 9801514 I added notes to the root module and the range set type to mention that SmallVec aspect of the container. You may declare a RangeSet with anything that implements the smallvec::Array trait, but in order to use it the item type must be a RangeInclusive<...> because all the impls have the additional constraint.
    In general the guideline for Rust is to only include type contraints on struct declarations if they will won't compile without them (don't ask me for the justification for this guideline, it's just what I've read, but for one it makes it easier if you decide later that some methods can work with more loose constraints). In this case the struct doesn't carry the T parameter at all, so it would probably require to use a PhantomData, if it could work at all. It's been a while since I did the initial implementation so I don't remember the issues I had.
  2. Yes that's the correct way to do it.
  3. If the SmallVec allocated space on the heap and now can fit all the remaining items in the on-stack array, then shrink_to_fit will de-allocate the unused heap space.
  4. That's true, set operations aren't implemented. I added a tracking issue here Add set operations #9

@mcclure
Copy link
Author

mcclure commented Dec 19, 2022

The new text is an improvement! I would recommend additionally adding a note to the RangeSet.iter doc (currently no doc at all) specifying it iterates over range members and suggesting the s.as_ref().iter() trick.

@spearman
Copy link
Owner

Added in 7263904

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants