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

try_len method #723

Merged
merged 5 commits into from Aug 9, 2023
Merged

try_len method #723

merged 5 commits into from Aug 9, 2023

Conversation

Easyoakland
Copy link
Contributor

@Easyoakland Easyoakland commented Aug 7, 2023

Add method to get the length of an iterator if the size_hint is consistent.
Reasons why this would be useful:

  1. It makes it more obvious when getting the length with it.try_len().unwrap() than with the match statement.
  2. Iterator adapters that increase the length of iteration are not supposed to implement ExactSizeIter as indicated here. This could be used in those cases.

@Philippe-Cholet
Copy link
Member

Philippe-Cholet commented Aug 7, 2023

Reading this, I remembered the size_hint() doc saying it should be fully trusted (bold below is mine):

size_hint() is primarily intended to be used for optimizations such as reserving space for the elements of the iterator, but must not be trusted to e.g., omit bounds checks in unsafe code. An incorrect implementation of size_hint() should not lead to memory safety violations.

So maybe the documentation of try_len should warn about an incorrect implementation of size_hint.

And to have a similar signatures to other functions in size_hint.rs (and not have &impl Iterator), I would do

fn try_len(sh: SizeHint) -> Option<usize> { ... }

then

size_hint::try_len(self.size_hint())

@Easyoakland
Copy link
Contributor Author

@Philippe-Cholet

So maybe the documentation of try_len should warn about an incorrect implementation of size_hint.
And to have a similar signatures to other functions in size_hint.rs (and not have &impl Iterator), I would do ...

Fixed in 952d8c4. If a length is needed for unsafe operations I believe one is supposed to bound by https://doc.rust-lang.org/std/iter/trait.TrustedLen.html

@phimuemue
Copy link
Member

Hi there. I see this might be handy in some circumstances.

  • Let's return Result<usize, (usize, Option<usize>)> so that the callers do not need to call size_hint a second time in the failure case.
  • Please inline size_hint::try_len.
  • I am unsure about the name. I guess that most users looking for something like this start out by searching for size_hint. Maybe unique_size_hint?
  • In the docs, I did not directly understand what "relies on" means. Maybe "inherits guarantees and restrictions from size_hint"?
  • Do we need the Sized trait bound?

@Easyoakland
Copy link
Contributor Author

Let's return Result<usize, (usize, Option)> so that the callers do not need to call size_hint a second time in the failure case.

Sure. I suppose it would save a re-computation if its useful in the error case.

Please inline size_hint::try_len.

Either you meant Itertools::try_len() or I've already done that. If you meant the former that's now done as well.
How does one determine when to inline a function? I've seen the #[inline] annotation used before but I haven't been able to figure out when to apply it. I've generally just used it when similar functions nearby also have it or if benchmarks show an improvement.

I am unsure about the name. I guess that most users looking for something like this start out by searching for size_hint. Maybe unique_size_hint?

I chose this name to mirror the naming as in ExactSizeIterator and here. The primary use case is for getting the length from chaining/adapting two iterators known to not overflow a usize or a specialization-like behavior on ExactSizeIterator in a function implementation.

In the docs, I did not directly understand what "relies on" means. Maybe "inherits guarantees and restrictions from size_hint"?

Sure.

Do we need the Sized trait bound?

Good catch. Originally it was needed because it incorrectly took impl Iterator. Now that it takes the sizeHint we don't need the bound.

@phimuemue
Copy link
Member

thank you.

Either you meant Itertools::try_len() or I've already done that.

I meant to literally eliminate size_hint::try_len by doing the computation directly in Itertools::try_len. As it stands, it only creates an indirection without real need.

Regarding ‘[inline]‘: you’re right, there’s no clear pattern in the code base, so let’s just not use it here.

@Easyoakland
Copy link
Contributor Author

I meant to literally eliminate size_hint::try_len by doing the computation directly in Itertools::try_len. As it stands, it only creates an indirection without real need.

Regarding ‘[inline]‘: you’re right, there’s no clear pattern in the code base, so let’s just not use it here.

fixed in 068de35

@phimuemue
Copy link
Member

Thanks.

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 9, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

  • bors build finished

@bors bors bot merged commit c5afdd3 into rust-itertools:master Aug 9, 2023
10 checks passed
@Easyoakland Easyoakland deleted the try_len branch August 14, 2023 06:21
@jswrenn jswrenn added this to the next milestone Nov 14, 2023
@jswrenn jswrenn mentioned this pull request Nov 14, 2023
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

Successfully merging this pull request may close these issues.

None yet

4 participants