Skip to content
This repository has been archived by the owner on Jul 31, 2018. It is now read-only.

001: initial C++ Streams proposal #2

Closed
wants to merge 2 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Dec 18, 2015

@trevnorris
Copy link
Contributor

/cc @bnoordhuis @rvagg Think your feedback would be valuable here as well.


Author: Fedor Indutny <fedor@indutny.com>
Status: draft
Date: Thu Dec 17 2015 19:39:46 GMT-0500 (EST)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider doing this how is done in LEPS (https://raw.githubusercontent.com/libuv/leps/master/001-remove-unix-support.md). As this is now all these entries will be placed on the same line.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this?

// **(optional)**
// Try to write provided data immediately without blocking, if not
// possible to do - should return `0` if no data was written, should decrement
// `count` and change pointers/length in `bufs` if some data was written.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we include a helper function or whatnot for recalculating the uv_buf_t**? We have the logic already in StreamWrap::DoTryWrite(), and it'd be a shame for everyone to need to implement basically the exact same on their own.

@chrisdickinson
Copy link

This might not be in-scope for this NEP, but one of the more interesting attributes of WHATWG streams is the ability to do brand-checking during readable.pipe(writable) and skip a round trip into JS if both the readable and writable sides can communicate at the C++ layer. It'd be interesting to expose this functionality through the public stream implementation — so that if a non-handle-backed Stream were piped to a handle-backed stream, things would still work, but if two handle-backed streams were .pipe()'d together, they could communicate directly.

@indutny
Copy link
Member Author

indutny commented Jan 12, 2016

Sorry for delay, I took it to consider things more carefully. Turns out, I agree with @bnoordhuis , adding this won't really create more value than problems. Let's close this for now.

Thank you for your time, and sorry for wasting it on this. 😞

@indutny indutny closed this Jan 12, 2016
@trevnorris
Copy link
Contributor

@indutny :( The concept was good. Hopefully we can figure out how to get something like this into core in the future.

@indutny
Copy link
Member Author

indutny commented Jan 14, 2016

We have this in core already :) I'm not suggesting to remove it, just don't think that making it public would be beneficial.

@trevnorris
Copy link
Contributor

Sorry. should have read "as public API in core"

@trevnorris
Copy link
Contributor

Forgot that even rejected proposals are still merged. Landed in 9faa231.

@mcollina
Copy link
Member

What's the status of this proposal in core? Was it exposed?

@indutny
Copy link
Member Author

indutny commented Jul 25, 2016

I believe the proposal was rejected. The APIs are not exposed.

@indutny indutny deleted the feature/stream-base branch July 25, 2016 16:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants