Adds partial_cmp.rs file and partial_cmp signature to mod.rs #268

assemblaj · 2019-10-01T02:32:14Z

Ports partial_cmp functionality from std::iter to async-std.

montekki · 2019-10-01T13:20:44Z

src/stream/stream/partial_cmp.rs

+{
+    type Output = Option<Ordering>;
+
+    fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {


uhhh, this doesn't look right. Values might arrive from streams at different points at time, but if this happens this doesn't mean that the streams are not equal. The more correct behaviour here would probably be as follows:

We cache both elements we are currently comparing in the state of the PartialCmpFuture, in Options.

As soon as both elements are received (Both options are .is_some()) we can compare elements.

We have to consider all edge cases: i.e.
a) we have l_next, l has closed, but we don't have r_next and r is not yet closed
b) the same for r_next
c) both streams are closed but we have only one element left in cache
d) and so on

I see. I wanted to start by replicating the std::iter code for partial_cmp as much as possible, but you're correct that it doesn't account for values from each stream arriving at different times. Thanks for the feedback.

I also think that fuseing both streams would help.

yoshuawuyts · 2019-10-01T14:59:56Z

src/stream/stream/partial_cmp.rs

+                        cx.waker().wake_by_ref();
+                        Poll::Pending
+                    },
+                    non_eq      => Poll::Ready(non_eq),


This is probably going to fail cargo fmt checks.

montekki · 2019-10-02T10:51:50Z

src/stream/stream/partial_cmp.rs

+        }
+
+        // wakes task to pull the next item from stream
+        cx.waker().wake_by_ref();


instead of this probably we should do a loop like in fold or skip_while

Generally, when should looping be used instead of cx.waker().wake_by_ref() ? I've seen either approach used throughout the codebase and I wasn't sure which was the most applicable for this situation.

montekki · 2019-10-02T10:53:02Z

src/stream/stream/partial_cmp.rs

+    L: Stream + Unpin + Sized,
+    R: Stream + Unpin + Sized,


I don't believe Unpin bounds are necessary here, are they?

montekki · 2019-10-02T10:56:25Z

src/stream/stream/mod.rs

+                .partial_cmp(vec![1.].into_iter().collect::<VecDeque<f64>>()).await;
+            let result_none = vec![std::f64::NAN].into_iter().collect::<VecDeque<f64>>()
+                .partial_cmp(vec![1.].into_iter().collect::<VecDeque<f64>>()).await;
+


Probably also tests for Greater and Less for streams of the same number of elements but with different values?

src/stream/stream/partial_cmp.rs

montekki · 2019-10-05T16:55:07Z

@yoshuawuyts Shall we r+?

yoshuawuyts

Thanks so much for this! All that's needed now I think is a rebase!

@montekki yes; let's def land this!

yoshuawuyts · 2019-10-11T20:51:49Z

@assemblaj it seems like the rebase didn't go as expected; this is changing 1800 lines and 79 files!

assemblaj · 2019-10-11T21:22:50Z

@yoshuawuyts I honestly have no idea what's happening here. Currently trying to fix this.

ghost · 2019-10-14T13:35:08Z

There are still some conflicts that need to be resolved.

Hides docs as standard in our codebase for all Future types Co-Authored-By: Fedor Sakharov <[email protected]>

This reverts commit aaf2638.

Hides docs as standard in our codebase for all Future types Co-Authored-By: Fedor Sakharov <[email protected]>

This reverts commit aaf2638.

assemblaj · 2019-10-14T14:24:37Z

@yoshuawuyts @stjepang Rebased again just now. Everything should be good.

ghost · 2019-10-14T15:12:28Z

It looks like map, for_each, and try_for_each are getting removed by this PR. I suppose that was not intentional?

assemblaj · 2019-10-14T15:49:22Z

@yoshuawuyts @stjepang The files for map, for_each, and try_for_each are there, they just keep getting removed from mod.rs for reason reason. There's been a lot of issues with this branch related to rebasing, so I created a new branch and opened a new PR here: #327

ghost · 2019-10-15T09:21:59Z

Thanks for fixing this and opening a new PR!

montekki reviewed Oct 1, 2019

View reviewed changes

yoshuawuyts reviewed Oct 1, 2019

View reviewed changes

montekki reviewed Oct 2, 2019

View reviewed changes

yoshuawuyts added the enhancement New feature or request label Oct 8, 2019

This was referenced Oct 9, 2019

Adds Stream:ge #285

Merged

[Draft PR] Adds Stream::gt #304

Merged

[Draft PR] Adds Stream::le #305

Closed

[Draft PR] Adds Stream::lt #306

Closed

yoshuawuyts approved these changes Oct 11, 2019

View reviewed changes

assemblaj force-pushed the assemblaj-partial_cmp branch from cdac349 to f8bd71c Compare October 11, 2019 21:34

assemblaj and others added 14 commits October 14, 2019 10:09

Adds partial_cmp.rs file and partial_cmp signature to mod.rs

9f604f9

Update src/stream/stream/partial_cmp.rs

f145485

Hides docs as standard in our codebase for all Future types Co-Authored-By: Fedor Sakharov <[email protected]>

adds tests that compare streams of same length

8e009d0

Replaces waker function with loop, reomves Unpin trait bounds

243c52a

cleans up examples

838f752

Adds latest version of relevant files

bfea84a

Revert "Adds latest version of relevant files"

5f67420

This reverts commit aaf2638.

removes new methods unrelated to branch

5317d4f

Adds partial_cmp.rs file and partial_cmp signature to mod.rs

1c0ee5f

Update src/stream/stream/partial_cmp.rs

815a36e

Hides docs as standard in our codebase for all Future types Co-Authored-By: Fedor Sakharov <[email protected]>

Replaces waker function with loop, reomves Unpin trait bounds

4370787

Revert "Adds latest version of relevant files"

ded4137

This reverts commit aaf2638.

removes new methods unrelated to branch

1fd8205

applies cargo fmt

17a3afb

assemblaj force-pushed the assemblaj-partial_cmp branch from d2c9c5f to 17a3afb Compare October 14, 2019 14:23

assemblaj mentioned this pull request Oct 14, 2019

Adds Stream::partial_cmp #327

Merged

ghost closed this Oct 15, 2019

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds partial_cmp.rs file and partial_cmp signature to mod.rs #268

Adds partial_cmp.rs file and partial_cmp signature to mod.rs #268

assemblaj commented Oct 1, 2019

montekki Oct 1, 2019

assemblaj Oct 1, 2019

montekki Oct 1, 2019 •

edited

Loading

yoshuawuyts Oct 1, 2019

montekki Oct 2, 2019

assemblaj Oct 2, 2019

montekki Oct 2, 2019

montekki Oct 2, 2019

montekki commented Oct 5, 2019

yoshuawuyts left a comment

yoshuawuyts commented Oct 11, 2019

assemblaj commented Oct 11, 2019

ghost commented Oct 14, 2019

assemblaj commented Oct 14, 2019

ghost commented Oct 14, 2019

assemblaj commented Oct 14, 2019

ghost commented Oct 15, 2019

Adds partial_cmp.rs file and partial_cmp signature to mod.rs #268

Adds partial_cmp.rs file and partial_cmp signature to mod.rs #268

Conversation

assemblaj commented Oct 1, 2019

montekki Oct 1, 2019

Choose a reason for hiding this comment

assemblaj Oct 1, 2019

Choose a reason for hiding this comment

montekki Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

yoshuawuyts Oct 1, 2019

Choose a reason for hiding this comment

montekki Oct 2, 2019

Choose a reason for hiding this comment

assemblaj Oct 2, 2019

Choose a reason for hiding this comment

montekki Oct 2, 2019

Choose a reason for hiding this comment

montekki Oct 2, 2019

Choose a reason for hiding this comment

montekki commented Oct 5, 2019

yoshuawuyts left a comment

Choose a reason for hiding this comment

yoshuawuyts commented Oct 11, 2019

assemblaj commented Oct 11, 2019

ghost commented Oct 14, 2019

assemblaj commented Oct 14, 2019

ghost commented Oct 14, 2019

assemblaj commented Oct 14, 2019

ghost commented Oct 15, 2019

montekki Oct 1, 2019 •

edited

Loading