Add Channel{T}(f::Function) constructors.

[NHDaly]

The existing Channel constructors are a bit inconvenient to work with. I love that you can create a Channel and spawn a task at the same time; it's super convenient! But the task-spawning constructor is a bit more cumbersome than the non-task-spawning constructor, because it takes its params as keyword arguments instead of the nicer way that the non-task constructor does it.

This PR adds two convenience constructors to combine those styles, to allow you to write things like this:

Channel{Char}(1) do chnl
    for c in "hello world"
        put!(chnl, c)
    end
end

---

The Channel constructors that do and don't create a Task are currently
written in different styles:

• The non-Task constructor takes a Type {T} as a type parameter, and
  the size as a positional argument.

  • Channel{Int}(Inf)
  • Channel(0)

• The Task constructor takes a Function, but the size and type are
  keyword arguments:

  • Channel(c->put!(c,0), ctype=Int, csize=0)
  • Channel(ctype=Int, csize=0, taskref=t) do c; put!(c,0); end

This commit adds convenience methods to the Task-creating constructor,
allowing you to specify the Type and (optionally) size as in the non-Task constructor:

• Channel{Char}(c->put!(c, 'a'))
• Channel{Char}(1) do c; put!(c, 'a'); end
• Channel{Char}(csize=2, taskref=t) do c; put!(c, 'a'); end
• Channel{Char}(c->put!(c, 'a'), Inf)

It also adds default constructors, defaulting to unbuffered channels (ie sz=0):

• Channel() = Channel(0)
• Channel{T}() where T = Channel{T}(0)

---

I also added tests for the existing Task-creating constructors, which weren't explicitly tested before.

---

EDIT: Some other things to consider:

• In this PR, I kept it so that the empty constructor, Channel() is not valid. I think that's probably still reasonable, although one could argue that this should simply use all the defaults -- ie Channel() = Channel{Any}(0). But I dunno, it's probably fine as-is.
• Let me know if the tests are reasonable.

[NHDaly]

This came up when i was working on adding examples to my CspExamples.jl repo, such as NHDaly/CspExamples.jl#2 and NHDaly/CspExamples.jl#4.

In the tests, it's a bit cumbersome to create new channels and fill them, which led me to want to add these constructors.

[vchuravy]

That looks like a good enhancement

[NHDaly]

Also, while we're here talking about this, I wonder if we should reevaluate that currently Channel{Int}() is disallowed. I think having this default to csize=0 might be beneficial. (And I guess relatedly then, Channel() would default to Channel{Any}(0).)

After reading a lot more about coroutines in Go, it sounds like you're almost always supposed to use an unbuffered channel (our Channel(0)), and the times where you need a buffered channel are rare and complex, and usually mean you're making things harder than they need to be. Since our constructor forces you to pick a number for the normal case, we leave people open to potential foot-guns. For example, before reading lots about it, I assumed that choosing Inf was a sane default unless I had reason to believe otherwise, thinking it seemed the "safest".

So I think this is a good opportunity to get ahead of potential bad practice. (That said, I'm fork this discussion into another issue/PR if you don't want to discuss it here, but this seems like as good a place as any! 😄)

[vchuravy]

Defaulting to unbuffered channels seems right to me.

…

On Mon, Jan 28, 2019, 18:43 Nathan Daly ***@***.*** wrote: Also, while we're here talking about this, I wonder if we should reevaluate that currently Channel{Int}() is disallowed. I think having this default to csize=0 might be beneficial. (And I guess relatedly then, Channel() would default to Channel{Any}(0).) After reading a lot more about coroutines in Go, it sounds like you're almost always supposed to use an unbuffered channel (our Channel(0)), and the times where you need a buffered channel are rare and complex, and usually mean you're making things harder than they need to be. Since our constructor forces you to pick a number for the normal case, we leave people open to potential foot-guns. For example, before reading lots about it, I assumed that choosing Inf was a sane default unless I had reason to believe otherwise, thinking it seemed the "safest". So I think this is a good opportunity to get ahead of potential bad practice. (That said, I'm fork this discussion into another issue/PR if you don't want to discuss it here, but this seems like as good a place as any! 😄) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#30855 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAI3aqAevxGxnvdhhQNrBLlxriXoTJczks5vH7VOgaJpZM4aUdc1> .

[NHDaly]

PTAL: This should be good to review again!

I have added compat statements for julia 1.3, and set the default Channel() constructor to default to unbuffered channels of Any.

[NHDaly]

Please take another look 😄

I know that feature-freeze for 1.3 is coming up soon, and I feel like since this is going to be the multithreading release, it would be nice to have a user-friendly interface on Channels.

Hopefully this can be resolved before feature freeze? :) Thanks!

[vchuravy]

Ah sorry should have been squashed -- mea culpa.

[JeffBezanson]

This causes a method overwrite warning. This signature can be Channel{T}(f::Function, sz).

[NHDaly]

Oops, sorry! I opened #32818 to fix it!

Thanks, good catch! Sorry I didn't notice (the error comes up while building and it's easy to miss. I'll keep an eye out for that in the future!) :)