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.

Sign up for GitHub

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 isgenerated on nightly #86

Merged
merged 1 commit into from
Jun 7, 2021
Merged

fix isgenerated on nightly #86

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/reflection/reflection.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@ using Core: CodeInfo, Typeof
using Core.Compiler: InferenceState, MethodInstance, svec
using InteractiveUtils: typesof

if isdefined(Base, :hasgenerator) # VERSION >= v"1.7.0"
Copy link
Member

Choose a reason for hiding this comment

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

Use the version check already defined later instead, the symbols can exist together with different meanings
You also don't want to shadow base functions when the if condition later can satisfy the usage anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Note that the check like this will not work for dev versions of 1.7. You can use contrib/commit-name.sh in the Julia repo to get the exact version number for a given PR. That said, the solution here also seems fairly reasonable to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to run the commit-name.sh script but I get the following error (sorry for the italian)

carlo@prestige julia]$ contrib/commit-name.sh 1bb560ba1cf83eab8a4eb0315a60142a0dc95382
fatal: il percorso 'VERSION' esiste su disco, ma non in '1bb560ba1cf83eab8a4eb0315a60142a0dc95382'
contrib/commit-name.sh: riga 13: [: troppi argomenti
fatal: bad object 1bb560ba1cf83eab8a4eb0315a60142a0dc95382
fatal: bad object 1bb560ba1cf83eab8a4eb0315a60142a0dc95382
contrib/commit-name.sh: riga 27: [: =: atteso operatore unario
contrib/commit-name.sh: riga 30: [: =: atteso operatore unario

Can we keep the isdefined since it doesn't make much difference? Also, I don't see any overshadowing problem, hasgenerator is not exported from Base, and since we use it in a couple of spots I prefer to do the isdefined check once instead of multiple times

Copy link
Member

Choose a reason for hiding this comment

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

You need to use the commit that was actually merged into master:

~/.../Julia/julia >>> ./contrib/commit-name.sh 2688a06
1.7.0-DEV.1096

But yeah, it's not really necessary here...

Copy link
Member

Choose a reason for hiding this comment

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

You could use the images that are built by the julia build bots for the PR too.

I think the necessary version checks VERSION >. ... are already in place?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand

Copy link
Member

Choose a reason for hiding this comment

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

Julia's build bots build the images for every pr, so we could potentially use those is what I meant.

Copy link
Member

Choose a reason for hiding this comment

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

Why not just use the nightly build? Or is that what you mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

can we stop discussing an issue that doesn't really exist and merge this? I'd like to have CI back on nightly

hasgenerator(x) = Base.hasgenerator(x)
else
hasgenerator(x) = Base.isgenerated(x)
end

worldcounter() = ccall(:jl_get_world_counter, UInt, ())

isprecompiling() = ccall(:jl_generating_output, Cint, ()) == 1
Expand Down Expand Up @@ -46,10 +52,10 @@ function meta(T; types = T, world = worldcounter())
sps = svec(map(untvar, sps)...)
@static if VERSION >= v"1.2-"
mi = Core.Compiler.specialize_method(method, types, sps)
ci = Base.isgenerated(mi) ? Core.Compiler.get_staged(mi) : Base.uncompressed_ast(method)
ci = hasgenerator(mi) ? Core.Compiler.get_staged(mi) : Base.uncompressed_ast(method)
else
mi = Core.Compiler.code_for_method(method, types, sps, world, false)
ci = Base.isgenerated(mi) ? Core.Compiler.get_staged(mi) : Base.uncompressed_ast(mi)
ci = hasgenerator(mi) ? Core.Compiler.get_staged(mi) : Base.uncompressed_ast(mi)
end
Base.Meta.partially_inline!(ci.code, [], method.sig, Any[sps...], 0, 0, :propagate)
Meta(method, mi, ci, method.nargs, sps)
Expand Down