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

Added multicaster stream #732

Merged
merged 4 commits into from Jul 18, 2014

Conversation

Projects
None yet
3 participants
@EricMCornelius

EricMCornelius commented Jul 16, 2014

An abstraction for reading from a single input and piping to various outputs

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 16, 2014

Member

Just one comment regarding the constructor: The outputs argument should be .duped, because at least on older versions of DMD, something like new MultiplexerStream(stream1, stream2) would silently allocate the outputs array on the stack and thus leads to memory corruption later on. Maybe this got fixed recently, but at least DMD 2.064 still has to be supported right now. Otherwise looks good to merge.

Member

s-ludwig commented Jul 16, 2014

Just one comment regarding the constructor: The outputs argument should be .duped, because at least on older versions of DMD, something like new MultiplexerStream(stream1, stream2) would silently allocate the outputs array on the stack and thus leads to memory corruption later on. Maybe this got fixed recently, but at least DMD 2.064 still has to be supported right now. Otherwise looks good to merge.

@etcimon

This comment has been minimized.

Show comment
Hide comment
@etcimon

etcimon Jul 17, 2014

Contributor

I think you're suggesting that this would be a demultiplexer, because multiplexing involves merging multiple streams into one. This seems to be more like stream broadcaster. I may be wrong about this, but the principle I've understood about a multiplexer is that it should write to one of many outputs exclusively depending on a predicate rather than do a raw copy of the entire stream to each sink.

Contributor

etcimon commented Jul 17, 2014

I think you're suggesting that this would be a demultiplexer, because multiplexing involves merging multiple streams into one. This seems to be more like stream broadcaster. I may be wrong about this, but the principle I've understood about a multiplexer is that it should write to one of many outputs exclusively depending on a predicate rather than do a raw copy of the entire stream to each sink.

@EricMCornelius EricMCornelius changed the title from Added multiplexer stream to Added broadcaster stream Jul 17, 2014

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jul 17, 2014

Member

@etcimon You are right of course. I think the most appropriate name would be MulticastStream, because broadcasting would mean to somehow send to all streams. Such a concept could maybe be implemented using a special BroadcastReceiverStream along with a channel ID, but that's a different thing.

@EricMCornelius Sorry for debating so much on details, but this is important to get right, so that we don't have to possibly break code later.

Member

s-ludwig commented Jul 17, 2014

@etcimon You are right of course. I think the most appropriate name would be MulticastStream, because broadcasting would mean to somehow send to all streams. Such a concept could maybe be implemented using a special BroadcastReceiverStream along with a channel ID, but that's a different thing.

@EricMCornelius Sorry for debating so much on details, but this is important to get right, so that we don't have to possibly break code later.

@EricMCornelius EricMCornelius changed the title from Added broadcaster stream to Added multicaster stream Jul 17, 2014

s-ludwig added a commit that referenced this pull request Jul 18, 2014

@s-ludwig s-ludwig merged commit 4ba9c59 into vibe-d:master Jul 18, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@EricMCornelius EricMCornelius deleted the EricMCornelius:multiplexer_stream branch Jul 18, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment