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

Reinstate printing of :line Exprs #15309

Merged
merged 1 commit into from
Mar 7, 2016
Merged

Reinstate printing of :line Exprs #15309

merged 1 commit into from
Mar 7, 2016

Conversation

toivoh
Copy link
Contributor

@toivoh toivoh commented Mar 1, 2016

Line numbers in ASTs are nowadays often represented with LineNumberNodes, but this at least doesn't hold for macro arguments, which caused trouble in #15299, and is demonstrated by the example

macro q(ex)
    QuoteNode([ex])
end
@q begin x end

which prints

1-element Array{Expr,1}:
 quote 
    $(Expr(:line, 1, :none))
    x
end

This patch reinstates printing of :line expressions as it existed in Julia 0.3.
An alternative (and maybe preferable?) fix would be to consistently present LineNumberNodes to the user instead.

@tkelman tkelman added the needs tests Unit tests are required for this change label Mar 1, 2016
@toivoh
Copy link
Contributor Author

toivoh commented Mar 2, 2016

@tkelman: Sure, I can add a few tests. Just wanted to know if this was at all desired.
But I guess that even if we would get rid of user-visible LineNumberNodes completely in 0.5, this could still bear back porting to 0.4.

@tkelman
Copy link
Contributor

tkelman commented Mar 2, 2016

Not sure. Was it bisected to see if the removal was intentional?

@toivoh
Copy link
Contributor Author

toivoh commented Mar 2, 2016

Seems to have been changed in 7bf3449, by @ihnorton. But the commit message doesn't say anything about :line Exprs.

@toivoh
Copy link
Contributor Author

toivoh commented Mar 2, 2016

Added some tests.

@tkelman tkelman removed the needs tests Unit tests are required for this change label Mar 2, 2016
@toivoh
Copy link
Contributor Author

toivoh commented Mar 3, 2016

The Travis OSX build passed, while all the others failed.
They all say that there was an error while loading /tmp/julia/share/julia/test/show.jl, in expression starting on line 369.
Weird, as far as I can tell, the expression starting on line 369 is

@test replace(str_ex2, " ", "") == replace("""
begin  # none, line 1:
    x
end""", " ", "")

Anybody have any idea why that might trigger a loading error? I'd been slightly less surprised if the few lines just above it did, since they involve a macro definition.

They are still given to the user in macro arguments,
otherwise LineNumberNode seems to be used consistently.
@toivoh
Copy link
Contributor Author

toivoh commented Mar 3, 2016

Sorry, silly mistake in the tests.
I was confused by the error message in the AppVeyor/Travis logs though:

LoadError: There was an error during testing

To me this would indicate that the test code caused an unexpected exception, not that a test failed. The actual failure message was out of sight one screen higher up, with output from a lot of other threads in between.

The test should hopefully pass this time. It seems that the reason that they passed on OSX was simply that the show.jl tests weren't run there.

@toivoh
Copy link
Contributor Author

toivoh commented Mar 4, 2016

AppVeyor 64 bit failed in float16:

Expression: round(Int,Float16(x)) == round(Int,x)
    From worker 10:    Evaluated: 0 == 1

Seems totally unrelated?

@tkelman
Copy link
Contributor

tkelman commented Mar 4, 2016

That is odd, and certainly unrelated.

@tkelman
Copy link
Contributor

tkelman commented Mar 4, 2016

I'm a little amazed x = Float32(rand()); round(Int,Float16(x)) == round(Int,x) hasn't failed sooner. Maybe random seed issue, as more tests gradually get added? For a bunch of Float32 values just above 0.5, Float16(x) returns Float16(0.5) which rounds to 0, but round(Int, x) rounds to 1.

@rfourquet
Copy link
Member

This rand test fails in approximately 0.025% of the runs.

@toivoh
Copy link
Contributor Author

toivoh commented Mar 4, 2016

Thanks for the info, @rfourquet!
If there's no objections, I'll merge this in a couple of days.

toivoh added a commit that referenced this pull request Mar 7, 2016
Reinstate printing of :line Exprs
@toivoh toivoh merged commit c12aead into master Mar 7, 2016
@toivoh toivoh deleted the toivoh-patch-1 branch March 7, 2016 17:40
@toivoh
Copy link
Contributor Author

toivoh commented Mar 10, 2016

Btw am I right that this would be a candidate for back porting to Julia 0.4? The problem exists on 0.4 as well, and I'd consider this a straight bug fix.

tkelman pushed a commit that referenced this pull request Mar 13, 2016
They are still given to the user in macro arguments,
otherwise LineNumberNode seems to be used consistently.

(cherry picked from commit 3a16b19)
ref #15309
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.

3 participants