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 ambiguous method warning printing #10984

Merged
merged 2 commits into from
May 5, 2015

Conversation

yuyichao
Copy link
Contributor

Warning printing before

julia> f(a::Any, b::Real) = 1
f (generic function with 1 method)

julia> f(a::Number, b::Any...) = 1
Warning: New definition 
    fTuple{Number,Vararg{Any}} at none:1
is ambiguous with: 
    fTuple{Any,Real} at none:1.
To fix, define 
    fTuple{Number,Real}
before the new definition.
f (generic function with 2 methods)

After

julia> f(a::Any, b::Real) = 1
f (generic function with 1 method)

julia> f(a::Number, b::Any...) = 1
Warning: New definition 
    f(Number, Any...) at none:1
is ambiguous with: 
    f(Any, Real) at none:1.
To fix, define 
    f(Number, Real)
before the new definition.
f (generic function with 2 methods)

@yuyichao
Copy link
Contributor Author

@yuyichao
Copy link
Contributor Author

This also happens to method override warning

Before

julia> module A; f() = 1; end; A.f() = 2;
Warning: Method definition fTuple{} in module A at none:1 overwritten in module Main at none:1.

After

julia> module A; f() = 1; end; A.f() = 2;
Warning: Method definition f() in module A at none:1 overwritten in module Main at none:1.

@yuyichao
Copy link
Contributor Author

So windows doesn't seem to be very happy with now stderr is captured in the test??

@JeffBezanson
Copy link
Member

Thanks for tackling this kind of tedious but very helpful fix. Could this share code with jl_ somehow? Some cases in there might also need this fix.

@yuyichao
Copy link
Contributor Author

@JeffBezanson I see. Just had a quick look and it seems that this is one of them.

I guess it's slightly less important since it's mainly for debugging but can still be quite annoy. Will include those fixes later (tonight).

Also, is there a straightforward way to capture the standard error of a process without having to spawn a external process (cat). Or should I redirect the stdout of the current process for the test?

@yuyichao
Copy link
Contributor Author

... Actually decided to fix it much earlier....

@yuyichao yuyichao force-pushed the ambiguous-warning branch 5 times, most recently from 0c4bc45 to 170d274 Compare April 25, 2015 14:32
@yuyichao
Copy link
Contributor Author

Confirm again on the CI (32bit 64bit) that the failure on windows seems to be cause by some bugs in redirection STDERR/capturing output. Disable the test on windows now to avoid pointless known failure.

@tkelman
Copy link
Contributor

tkelman commented Apr 26, 2015

I think this is worth squashing into 2 commits - everything except the last commit, then keep the last commit separate.

We should open a separate issue to track fixing that test on Windows.

@tkelman
Copy link
Contributor

tkelman commented Apr 26, 2015

On second thought it may have been fixed by 456c222, worth rebasing to check.

@yuyichao
Copy link
Contributor Author

OK, I'll keep the last commit on another branch and squash the rest to check.

@yuyichao
Copy link
Contributor Author

Hmm, so looks like the failure is still there? I'll include the last commit then.

Not sure if/how I should report this though since I don't have a windows machine to reproduce it locally. (Or maybe I should just point to the CI result here?)

@tkelman
Copy link
Contributor

tkelman commented Apr 26, 2015

(Or maybe I should just point to the CI result here?)

That'll do fine. Just want to track the problem somewhere permanent so we don't forget about it until it's fixed.

@yuyichao
Copy link
Contributor Author

yuyichao commented May 5, 2015

Ping.

Can someone please review this or does anyone have any comment on what I should improve (or what I still miss)?

jakebolewski added a commit that referenced this pull request May 5, 2015
Fix ambiguous method warning printing
@jakebolewski jakebolewski merged commit 06965c4 into JuliaLang:master May 5, 2015
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.

4 participants