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

Fix wrong use of isdefined #21492 #21539

Merged
merged 2 commits into from
Apr 26, 2017
Merged

Fix wrong use of isdefined #21492 #21539

merged 2 commits into from
Apr 26, 2017

Conversation

vchuravy
Copy link
Sponsor Member

This is the immediate fix from #21537.

`isdefined` will immediatly resolve the binding. Since we are only
trying to skip the path lookup the weaker version `isbindingresolved`
is enough. `isbindingresolved` is also used for this purpose in
`read_verify_mod_list` in `src/dump.c`.
@tkelman
Copy link
Contributor

tkelman commented Apr 25, 2017

any quick and/or simple within-base way of testing this?

@vchuravy
Copy link
Sponsor Member Author

I have been thinking about it, but nothing that is simple and wouldn't poison the test worker.

@amitmurthy
Copy link
Contributor

You can have a test that runs outside of the main workers set. See how boundscheck_exec.jl and distributed_exec.jl are run.

@vchuravy
Copy link
Sponsor Member Author

thanks that pointed me into the right direction.

@fredrikekre
Copy link
Member

(this fixes the problems I had in #21492 (comment), thanks!)

@vchuravy
Copy link
Sponsor Member Author

@tkelman Is the test sufficient? My only worry is that it will stop testing what it needs to test if Base.Iterators changes.

@tkelman
Copy link
Contributor

tkelman commented Apr 25, 2017

better than no test!

(should be 4 space indent though)

@vchuravy
Copy link
Sponsor Member Author

vchuravy commented Apr 25, 2017 via email

@vchuravy
Copy link
Sponsor Member Author

@vtjnash Good to merge?

@vtjnash vtjnash merged commit 98414b9 into master Apr 26, 2017
@vtjnash vtjnash deleted the vc/fix21492 branch April 26, 2017 01:38
@tkelman
Copy link
Contributor

tkelman commented Apr 26, 2017

Is the WARNING: replacing module Test6c92f26 expected here? If so, should it use @test_warn to suppress and check for that?

@vchuravy
Copy link
Sponsor Member Author

vchuravy commented Apr 26, 2017 via email

@tkelman
Copy link
Contributor

tkelman commented Apr 26, 2017

set its stdout and/or stderr?

tkelman pushed a commit that referenced this pull request May 3, 2017
`isdefined` will immediatly resolve the binding. Since we are only
trying to skip the path lookup the weaker version `isbindingresolved`
is enough. `isbindingresolved` is also used for this purpose in
`read_verify_mod_list` in `src/dump.c`.

(cherry picked from commit 6c92f26)
ref #21539
tkelman pushed a commit that referenced this pull request May 3, 2017
(cherry picked from commit cd227e9)
ref #21539
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