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

RFC: remove deprecation about __precompile__(true) #28459

Merged
merged 1 commit into from
Aug 6, 2018

Conversation

KristofferC
Copy link
Sponsor Member

Doesn't seem to be much value in deprecating this?

@StefanKarpinski StefanKarpinski added kind:deprecation This change introduces or involves a deprecation and removed kind:deprecation This change introduces or involves a deprecation labels Aug 5, 2018
base/loading.jl Outdated
if isprecompilable
depwarn("__precompile__() is now the default", :__precompile__)
elseif 0 != ccall(:jl_generating_output, Cint, ())
if ccall(:jl_generating_output, Cint, ()) != 0
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

&& !isprecompilable ?

base/loading.jl Outdated

Specify that the file calling this function is not precompilable.
Specify that the file calling this function is not precompilable, defaulting to `true`.
Copy link
Member

Choose a reason for hiding this comment

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

The wording makes it sound like precompilation is disabled by default. I'd reword as

Specify whether the file calling this function is precompilable, defaulting to true. If this function is not called explicitly, precompilation will be enabled by default.

@KristofferC
Copy link
Sponsor Member Author

Updated.

Copy link
Member

@ararslan ararslan left a comment

Choose a reason for hiding this comment

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

It'd be nice to mention in the docstring that not using the function at all defaults to precompiling, but that doesn't have to happen here.

@KristofferC
Copy link
Sponsor Member Author

Yeah, that's a good point.

@ararslan ararslan mentioned this pull request Aug 6, 2018
3 tasks
@ararslan
Copy link
Member

ararslan commented Aug 6, 2018

Thinking about this more, isn't the deprecation message fine to have for 0.7? It will tell people what the behavior will be in 1.0 before we silently break it.

@JeffBezanson
Copy link
Sponsor Member

That would be a different message though --- in the case where there's no __precompile__ call, not where there is a __precompile__() call.

@ararslan
Copy link
Member

ararslan commented Aug 6, 2018

I see. It seems definitely worthwhile to me to emit a warning when there's no __precompile__ call at all.

@KristofferC
Copy link
Sponsor Member Author

Separate PR? :)

@ararslan
Copy link
Member

ararslan commented Aug 6, 2018

Yeah definitely

@JeffBezanson JeffBezanson merged commit e3bc48d into master Aug 6, 2018
@JeffBezanson JeffBezanson deleted the kc/precompile_dep branch August 6, 2018 16:04
@Keno
Copy link
Member

Keno commented Aug 6, 2018

I see. It seems definitely worthwhile to me to emit a warning when there's no precompile call at all.

I don't think we should do this. It's perfectly legal to have no precompile call now and the previous depwarn has caused people to remove theirs.

@KristofferC
Copy link
Sponsor Member Author

KristofferC commented Aug 6, 2018

the previous depwarn has caused people to remove theirs.

Yeah, the deprecation should have deprecated having no precompile statement forcing people to do __precompile__(false) while leaving __precompile__(true) alone. Forcing the people that removed their precompile statement to add them back seems quite cruel though.

@JeffBezanson
Copy link
Sponsor Member

Agreed. I also think most/all packages that don't support precompilation have already marked __precompile__(false).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants