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

replace number parsing functions with parse and tryparse #10543

Merged
merged 7 commits into from
Mar 17, 2015
Merged

Conversation

JeffBezanson
Copy link
Sponsor Member

Continuation of #9487

tanmaykm and others added 4 commits March 17, 2015 21:57
Introduces the tryparse method:
- tryparse{T<:Integer}(::Type{T<:Integer},s::AbstractString)
- tryparse(::Type{Float..},s::AbstractString)
- a few variants of the above

And:
- tryparse(Float.., ...) call the corresponding C functions jl_try_strtof, jl_try_substrtof, jl_try_strtod and jl_try_substrtod.
- The parseint, parsefloat, float64_isvalid and float32_isvalid methods wrap the corresponding tryparse methods.
- The jl_strtod, jl_strtof, ... functions are wrappers over the jl_try_str... functions.

This should fix #10498 as well.

Ref: discussions at #9316, #3631, #5704
I'd rather isolate BigNum code as much as possible
tf = tryparse(T, s)
isnull(tf) || (out[1] = get(tf))
!isnull(tf)
end
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Why no depwarn here?

A comment refering to #10543 would also be great so that it is easier to see where a deprecation is comming from (without digging into git history).

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

This is actually just a utility used by actual deprecated functions. There never was a float_isvalid before.

Copy link

Choose a reason for hiding this comment

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

Unrelated question, in this function isnull is called twice, would it in
general offer a performance gain to save the value from the first call in a
local variable or is the execution time equivalent to that of fetching the
variable? Is there a definable turning point in function complexity at
which saving its output becomes beneficial? Can saving array values as
local variables offer improvement or does Julia automatically detect and
optimize reusable local values?
On Mar 17, 2015 8:01 PM, "Jeff Bezanson" notifications@github.com wrote:

In base/deprecated.jl
#10543 (comment):

@@ -496,3 +496,19 @@ function to_index{T<:Real}(A::AbstractArray{T})
depwarn("indexing with non Integer AbstractArrays is deprecated", :to_index)
Int[to_index_nodep(x) for x in A]
end
+
+function float_isvalid{T<:Union(Float32,Float64)}(s::AbstractString, out::Array{T,1})

  • tf = tryparse(T, s)
  • isnull(tf) || (out[1] = get(tf))
  • !isnull(tf)
    +end

This is actually just a utility used by actual deprecated functions. There
never was a float_isvalid before.


Reply to this email directly or view it on GitHub
https://github.com/JuliaLang/julia/pull/10543/files#r26598614.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Yes, in general re-using a result is faster than calling a function twice.
However in this case, isnull is so trivial:

isnull(x::Nullable) = x.isnull

that it will be inlined and I'm pretty sure there'd be nothing to gain by saving its result.

In a very small number of cases Julia can automatically avoid redundant work from calling a function multiple times with the same arguments. However this is hard to do in general. For example Arrays are mutable, so it's meaningful to allocate two different ones with the same contents --- you might be planning to mutate them differently. So we can't do this optimization in such cases. Much more can be said but this topic gets complicated quickly.

@JeffBezanson
Copy link
Sponsor Member Author

Ah, net -3 functions. That's what I like to see.

We could also replace the raise argument to the existing parse with tryparse.

@johnmyleswhite
Copy link
Member

Love this change.

@jakebolewski
Copy link
Member

+1 to tryparse

JeffBezanson added a commit that referenced this pull request Mar 17, 2015
replace number parsing functions with parse and tryparse
@JeffBezanson JeffBezanson merged commit 7798fda into master Mar 17, 2015
@JeffBezanson JeffBezanson deleted the tryparse branch March 18, 2015 02:31
@tkelman
Copy link
Contributor

tkelman commented Mar 19, 2015

Since float32_isvalid and float64_isvalid were deprecated without using the @deprecate macro, they need to be put back in the exports list.

@quinnj
Copy link
Member

quinnj commented Apr 6, 2015

What would be the best way to do 0.3 compatibility with tryparse (since I'm assuming backporting wouldn't be a possibility)? Something like:

function tryparse{T}(::Type{T}, x)
    try
        v = Nullable(parse(T, x))
    catch
        v = Nullable{T}()
    end
    return v
end

? Not quite sure all that was change here (or if there's something more performant for compatibility)

@JeffBezanson
Copy link
Sponsor Member Author

Yes that should work.

@quinnj
Copy link
Member

quinnj commented Apr 10, 2015

I ended up with the following, should probably submit to Compat

function tryparse{T<:Integer}(::Type{T}, x)
        local v
        try
            v = Nullable(parseint(T,x))
        catch
            v = Nullable{T}()
        end
        return v
    end
    function tryparse{T<:FloatingPoint}(::Type{T},x)
       local v
        try
            v = Nullable(parsefloat(T,x))
        catch
            v = Nullable{T}()
        end
        return v 
    end
    function tryparse{T}(::Type{T},x)
        local v
        try
            v = Nullable(parse(x)::T)
        catch
            v = Nullable{T}()
        end
        return v
    end

@tkelman
Copy link
Contributor

tkelman commented Apr 10, 2015

Could you check VERSION instead of using a try/catch?

@quinnj
Copy link
Member

quinnj commented Apr 10, 2015

Not sure I follow @tkelman. I'm trying to get a package working on 0.3 and 0.4 and utilize the tryparse in 0.4, so I need something that will emulate the behavior in 0.3.

@tkelman
Copy link
Contributor

tkelman commented Apr 10, 2015

The way almost all of the Compat.jl "use 0.4 syntax on 0.3" replacement rules work is by checking the exact VERSION where the change was made on 0.4-dev, and only applying the replacement rule for versions before that.

@quinnj
Copy link
Member

quinnj commented Apr 10, 2015

Sure, I understand that and have that kind of VERSION check already. My question revolves around what is the best replacement form for 0.3.

@tkelman
Copy link
Contributor

tkelman commented Apr 10, 2015

Right, nevermind, sorry I missed that the semantics of this would appear to require a try/catch on 0.3.

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.

None yet

9 participants