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

Always define range(start, stop) #642

Merged
merged 2 commits into from
Jan 25, 2019
Merged

Conversation

martinholters
Copy link
Member

Ref. #640 and JuliaLang/julia#30374 (comment). I aim at implementing option 3 mentioned in the latter, i.e. tag a release after this has been merged, then remove the offending functionality and tag another release.

Downside here is

WARNING: Method definition range(Any, Any) in module Base at range.jl:91 overwritten in module Compat at /home/travis/build/JuliaLang/Compat.jl/src/Compat.jl:1857.
WARNING: Method definition #range(Any, typeof(Base.range), Any, Any) in module Base overwritten in module Compat.

of course.

@martinholters
Copy link
Member Author

Well, the warning could be avoided with

function __init__()
    @eval Base begin
        range(start, stop; kwargs...) = range(start; stop=stop, kwargs...)
    end
end

but that feels so wrong I'm not sure I don't prefer the warning... Better ideas?

@martinholters martinholters force-pushed the mh/always_define_range branch 3 times, most recently from c914edc to 20e5090 Compare January 22, 2019 07:43
@martinholters
Copy link
Member Author

Thinking about this again, it would be nice to deprecate range(start, stop) in the next major version (instead of making it error directly), but then these warnings would be really unfortunate, so I've updated with the @eval Base version.

@ararslan, any comments?

if VERSION < v"1.1.0-DEV.506"
range(start, stop; kwargs...) = range(start; stop=stop, kwargs...)
else
function __init__()
@eval Base begin
Copy link
Member

Choose a reason for hiding this comment

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

This is really heinous... I think at one point @yuyichao mentioned that it's bad for precompilation as well, though I fully admit to not understanding the details on why.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doing @eval Base outside __init__ runs afoul of precompilation (and gives a warning to that effect).

Copy link
Member

Choose a reason for hiding this comment

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

So it's legit to do in __init__, just not in the package body?

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'd hope so (to the extent it's ever legit to eval into Base).

Copy link
Member

Choose a reason for hiding this comment

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

to the extent it's ever legit to eval into Base

...which I thought was never

Copy link
Member Author

Choose a reason for hiding this comment

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

In any case, I haven't found a better way of achieving the (short-term) goal of providing range(start, stop) on Julia 1.1.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm hesitant about this approach but I don't have a better suggestion.

Copy link
Member Author

@martinholters martinholters Jan 22, 2019

Choose a reason for hiding this comment

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

Great, so we're in agreement here. Who else might have something to contribute here that could bring us to a decision?

Copy link
Member Author

Choose a reason for hiding this comment

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

@StefanKarpinski, JuliaLang/julia#30374 (comment) means you have to chime in here 😄

@martinholters
Copy link
Member Author

Barring objections, I think I'll merge this tomorrow. Even if the implementation has flaws (and I'm not saying it has, just that it might), it's still a step forward IMO.

@martinholters martinholters merged commit 64697fd into master Jan 25, 2019
@martinholters martinholters deleted the mh/always_define_range branch January 25, 2019 12:23
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.

2 participants