Skip to content

Conversation

@ranjanan
Copy link
Contributor

Fixes #223 .

src/Compat.jl Outdated
end
export @threads
const threadid() = 1
const nthreads() = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

No const

test/runtests.jl Outdated
end

# Issue #223
if VERSION < v"0.5"
Copy link
Contributor

Choose a reason for hiding this comment

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

Fwiw this condition is always true even on master.

Copy link
Contributor Author

@ranjanan ranjanan Jun 28, 2016

Choose a reason for hiding this comment

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

Yeah, I'll change this to VERSION < v"0.5.0"

@yuyichao
Copy link
Contributor

Maybe aa well just relax the test

@ranjanan
Copy link
Contributor Author

@yuyichao You meant not have the test? Because in v0.5, you can start Julia with more than one thread. Then Compat tests would fail.

@yuyichao
Copy link
Contributor

No i mean sth like just making sure both exist and 0 < id < n

@ranjanan
Copy link
Contributor Author

Umm, but what would n be? Shall I simply say nthreads() >= 1 and threadid() == 1 and relax the condition?

@yuyichao
Copy link
Contributor

n was short for nthreads (and the second < was supposed to be <=) we can do 1 == threadid() <= nthreads() too if we want to guarantee that the test will be run on the first thread.

@yuyichao
Copy link
Contributor

Also, I think v"0.5.0" === v"0.5"

@ranjanan
Copy link
Contributor Author

Okay, I'll simply remove the condition and do 1 == threadid() >= nthreads()

@ranjanan
Copy link
Contributor Author

@yuyichao I made the same mistake again. Could you please switch off the CI for the previous commits and enable it for the latest commit? Thanks.

@yuyichao
Copy link
Contributor

I canceled the extra travis build. I can't do that for AppVeyor ones but I think that one has auto detection.

Anyway, this LGTM now.


# Issue #223
@test 1 == threadid() <= nthreads()

Copy link
Contributor

Choose a reason for hiding this comment

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

Need import or qualified name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the bug is in a previous line.

@ranjanan
Copy link
Contributor Author

It seems tryparse(Int32, "0", 1) does not throw an error. It used to throw an ArgumentError on v0.4? Is this expected behavior on v0.5?

@yuyichao
Copy link
Contributor

Probably JuliaLang/julia#17078 @quinnj

@ranjanan
Copy link
Contributor Author

ranjanan commented Jul 11, 2016

@yuyichao I see that JuliaLang/julia#17333 has been merged. Could you restart the build for this PR please?

@yuyichao
Copy link
Contributor

I can't restart the appveyor here so it's easier if you can rebase this pr.

@tkelman tkelman closed this Jul 11, 2016
@tkelman tkelman reopened this Jul 11, 2016
@TotalVerb
Copy link
Contributor

New failure should be fixed by #249.

@TotalVerb TotalVerb closed this Jul 12, 2016
@TotalVerb TotalVerb reopened this Jul 12, 2016
@stevengj stevengj merged commit 12c36d7 into JuliaLang:master Jul 12, 2016
dpsanders pushed a commit to dpsanders/Compat.jl that referenced this pull request Feb 1, 2017
* Add `threadid()` and `nthreads()`

* Fix `const` in definition

* Fix condition in test

* Remove condition and modify test

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants