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

in Channel put!, convert value to Channel type #29092

Merged
merged 1 commit into from
Oct 20, 2018

Conversation

tanmaykm
Copy link
Member

@tanmaykm tanmaykm commented Sep 8, 2018

put!(ch::Channel{T}, v) should convert v to type T.

This can prevent errors like:

julia> c = Channel{Int}(0)
Channel{Int64}(sz_max:0,sz_curr:0)

julia> @async put!(c, :a)
Task (runnable) @0x00007ff5b9d79270

julia> isready(c) && take!(c)
ERROR: TypeError: in take_unbuffered, in typeassert, expected Int64, got Symbol
Stacktrace:
 [1] take_unbuffered(::Channel{Int64}) at ./channels.jl:323
 [2] take!(::Channel{Int64}) at ./channels.jl:306
 [3] top-level scope at none:0

@vchuravy vchuravy added the needs tests Unit tests are required for this change label Sep 8, 2018
base/channels.jl Outdated
check_channel_state(c)
isbuffered(c) ? put_buffered(c,v) : put_unbuffered(c,v)
vT = convert(T, v)
Copy link
Member

Choose a reason for hiding this comment

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

It's no longer a performance issue to reuse the name v for this. Style is a different question, but I thought I'd mention it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I was partly skeptical when I did this. Will add a commit to reuse name.

`put!(ch::Channel{T}, v)` should convert `v` to type `T`.

This can prevent errors like:

```
julia> c = Channel{Int}(0)
Channel{Int64}(sz_max:0,sz_curr:0)

julia> @async put!(c, :a)
Task (runnable) @0x00007ff5b9d79270

julia> isready(c) && take!(c)
ERROR: TypeError: in take_unbuffered, in typeassert, expected Int64, got Symbol
Stacktrace:
 [1] take_unbuffered(::Channel{Int64}) at ./channels.jl:323
 [2] take!(::Channel{Int64}) at ./channels.jl:306
 [3] top-level scope at none:0
```
@tanmaykm
Copy link
Member Author

added tests

@JeffBezanson JeffBezanson added bugfix This change fixes an existing bug triage This should be discussed on a triage call and removed needs tests Unit tests are required for this change labels Sep 12, 2018
@JeffBezanson
Copy link
Member

I think this is a bugfix, but will see what triage thinks about backporting.

@StefanKarpinski
Copy link
Member

I'd be curious to hear what @DatName's objection to this is.

@DatName
Copy link

DatName commented Sep 13, 2018

@StefanKarpinski I think, that if you are explicitly creating a channel for Int, you should not be putting some other stuff in there silently. It is easy to do a convertion explicitly on user's side, if needed, so no need for this "behind-the-curtains-magic", so to speak.

@StefanKarpinski
Copy link
Member

I see your point of view. However, we do automatic conversion when values are put into typed contexts of all kinds already. Our conversions are quite strict about what conversions they will do without error: the value must be convertible exactly or an InexactError will be thrown.

@JeffBezanson
Copy link
Member

For values that aren't convertible (e.g. a string) this gives an error sooner than we did before.

@JeffBezanson JeffBezanson removed the bugfix This change fixes an existing bug label Sep 13, 2018
@JeffBezanson JeffBezanson added this to the 1.1 milestone Sep 13, 2018
@JeffBezanson JeffBezanson added minor change Marginal behavior change acceptable for a minor release and removed triage This should be discussed on a triage call labels Sep 13, 2018
@JeffBezanson JeffBezanson merged commit 74657ec into JuliaLang:master Oct 20, 2018
mortenpi added a commit to mortenpi/julia that referenced this pull request Dec 1, 2018
fredrikekre added a commit that referenced this pull request Dec 5, 2018
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <[email protected]>
Co-authored-by: Fredrik Ekre <[email protected]>
fredrikekre added a commit that referenced this pull request Dec 5, 2018
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <[email protected]>
Co-authored-by: Fredrik Ekre <[email protected]>
fredrikekre added a commit that referenced this pull request Dec 5, 2018
Addition of NEWS and compat admonitions for important changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.


Co-authored-by: Morten Piibeleht <[email protected]>
Co-authored-by: Fredrik Ekre <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor change Marginal behavior change acceptable for a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants