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

Index access depreciated #713

Merged
merged 5 commits into from Nov 7, 2017

Conversation

Projects
None yet
4 participants
@AndyGauge
Copy link
Contributor

AndyGauge commented Aug 22, 2017

Fixes #697

@ahmedcharles

This comment has been minimized.

Copy link
Collaborator

ahmedcharles commented Aug 23, 2017

It should be possible to provide an impl of ExactSizeIterator which would retain access to the len function.

@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Aug 23, 2017

Ah, specifically I do not want the exact size to be exposed. This allows the iterator to skip or insert events during iteration.

src/poll.rs Outdated
@@ -1161,7 +1161,7 @@ impl Poll {
self.readiness_queue.poll(&mut events.inner);

// Return number of polled events
Ok(events.len())
Ok(events.iter().count())

This comment has been minimized.

@AndyGauge

AndyGauge Aug 23, 2017

Author Contributor

Should we not return the number of polled events since we do not know the exact number?

This comment has been minimized.

@carllerche

carllerche Aug 23, 2017

Owner

Yeah, we should definitely not return the number... We have to leave it for now, but that return value should be deprecated.

This comment has been minimized.

@AndyGauge

AndyGauge Aug 23, 2017

Author Contributor

What about a 0 or 1? Return 0 for no events (like a timeout) and 1 if there is at least one?

This comment has been minimized.

@carllerche

carllerche Aug 31, 2017

Owner

Yeah, I think some return value like that could be good, but it will have to be punted until 0.7. We can open an issue to track that change. The ret value today will have to stay the same :(

AndyGauge added some commits Sep 1, 2017

@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Sep 1, 2017

Thanks! Looks good. Would you mind also updating the documentation for Poll::poll to indicate that the usize return variable is deprecated and will be removed in 0.7?

@AndyGauge

This comment has been minimized.

Copy link
Contributor Author

AndyGauge commented Sep 1, 2017

Sure, once the tests finish I'll get that done. I meant to ask how to deprecate the return value!

@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Sep 1, 2017

@AndyGauge I think it has to just be part of the fn docs as well as do a pass of the rest of the docs / tests to ensure it isn't being used.

Maybe @alexcrichton has a better idea?

@carllerche carllerche referenced this pull request Sep 1, 2017

Closed

Remove Events::get #697

@alexcrichton

This comment has been minimized.

Copy link
Contributor

alexcrichton commented Sep 1, 2017

Nah yeah there's no way I know of to deprecate just the return value, but you could temporarily switch it to a bool and that'd flag every location that needs to be fixed up internally at least

@AndyGauge

This comment has been minimized.

Copy link
Contributor Author

AndyGauge commented Sep 14, 2017

Found an example that needs to be reworked. Poll::deregister example (poll.rs#892)

    /// // Set a timeout because this poll should never receive any events.
    /// let n = poll.poll(&mut events, Some(Duration::from_secs(1)))?;
    /// assert_eq!(0, n);

Looking for high level guidance how this should be addressed.

@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Sep 15, 2017

I think checking for zero right now is OK. It's the only way to test if poll returned due to event OR due to timeout. When the return value is changed, it will have some equivalent API to check if n == 0.

@AndyGauge

This comment has been minimized.

Copy link
Contributor Author

AndyGauge commented Sep 15, 2017

That's why I wanted to bring it to your attention. We likely need to create a convenience for this case like .empty or whatever makes sense for your API

@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Sep 15, 2017

Ah, so basically add a test fn on the Events instead of rely on the return value. I think that might be an interesting idea.

@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Nov 7, 2017

Ok, I think the answer will be to add Events::is_empty().

This PR is good as is, so I'll create a tracking issue for this.

@carllerche carllerche merged commit 92ac500 into carllerche:master Nov 7, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.