Make `parse(T, "")` and `parse(T, " ")` etc, more specific regarding whitespace and empty strings #20588

Tetralux · 2017-02-12T23:03:37Z

Fixes #20587.

Before this PR:

julia> parse(Int, "", 16)
ERROR: ArgumentError: invalid base: base must be 2 ≤ base ≤ 62, got 0
Stacktrace:
 [1] tryparse_internal(::Type{Int64}, ::String, ::Int64, ::Int64, ::Int64, ::Bool) at ./parse.jl:63
 [2] parse(::Type{Int64}, ::String, ::Int64) at ./parse.jl:146

After this PR:

julia> parse(Int, "", 16)
ERROR: ArgumentError: input string is empty or only contains whitespace
Stacktrace:
 [1] tryparse_internal(::Type{Int64}, ::String, ::Int64, ::Int64, ::Int64, ::Bool) at .\parse.jl:62
 [2] parse(::Type{Int64}, ::String, ::Int64) at .\parse.jl:161

This message will also appear for parse(T, "") and parse(T, " "), etc.

…ntains only whitespace

JeffBezanson · 2017-02-12T23:14:14Z

base/parse.jl

@@ -58,6 +58,10 @@ end

 function tryparse_internal{T<:Integer}(::Type{T}, s::AbstractString, startpos::Int, endpos::Int, base_::Integer, raise::Bool)
 _n = Nullable{T}()
+ if isempty(strip(s))


I think calling strip will be too expensive here --- this function is very performance-critical, and most inputs don't need strip at all. I believe parseint_preamble returns a base of 0 if there is a problem; we should check for all-whitespace strings only in that case.

Should have spotted that. Will commit shortly.

Tetralux · 2017-02-13T01:13:27Z

Sorry that took a while. Took me farting around for an hour, trying to make it better for Bool, only to realise there was a much simpler way of doing it. >D

JeffBezanson · 2017-02-13T02:32:31Z

base/parse.jl

- Nullable{Bool}()
+
+ substr = SubString(sbuff, startpos, endpos)
+ if count(isspace, sbuff) == len # all chars were whitespace


I'm not too familiar with this code, but should this be checking substr instead of sbuff?

I should probably add a test for that. >)

Tetralux · 2017-02-13T02:57:03Z

Third time's a charm.

tkelman · 2017-02-13T04:12:57Z

@nanosoldier runbenchmarks(ALL, vs = ":master")

nanosoldier · 2017-02-13T07:23:56Z

Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @jrevels

nalimilan · 2017-02-13T08:54:01Z

base/parse.jl

@@ -126,27 +130,49 @@ end

 function tryparse_internal(::Type{Bool}, sbuff::Union{String,SubString},
 startpos::Int, endpos::Int, base::Integer, raise::Bool)
- len = endpos-startpos+1
- p = pointer(sbuff)+startpos-1
+ null = Nullable{Bool}()


I don't really see the point of defining this variable. Why not write Nullable{Bool}() where you need it?

nalimilan · 2017-02-13T09:04:52Z

base/parse.jl

+
+ substr = SubString(sbuff, startpos, endpos)
+ if count(isspace, substr) == len # all chars were whitespace
+ raise && throw(ArgumentError("input string only contains whitespace"))


This looks a bit weird stylistically. Why not wrap both branches inside an if raise block?

nalimilan · 2017-02-13T09:08:49Z

base/parse.jl

- Nullable{Bool}()
+
+ substr = SubString(sbuff, startpos, endpos)
+ if count(isspace, substr) == len # all chars were whitespace


all(isspace, substr) would be simpler (if I'm not missing something).

nalimilan · 2017-02-13T09:10:07Z

base/parse.jl

+ if isnull(result)
+ throw(ArgumentError("cannot parse $(repr(s)) as $T"))
+ end
+ return get(result)


You can use unsafe_get since you have already checked isnull.

nalimilan · 2017-02-13T09:11:02Z

test/parse.jl

+# Issue 20587
+for T in vcat(subtypes(Signed), subtypes(Unsigned))
+ for s in ["", " ", " "]
+ # Without a base (handles things like "0x00001111", etc)


Remove one space here.

Sorry - from where?

There are 5 spaces instead of 4.

nalimilan · 2017-02-13T09:11:19Z

test/parse.jl

+
+ # Test `tryparse_internal` with part of a string
+ let b = " "
+ result = @test_throws ArgumentError get(Base.tryparse_internal(Bool, b, 7, 11, 0, true)) == true


== true is redundant (same below).

nalimilan · 2017-02-13T09:12:21Z

test/parse.jl

+
+ # With a base
+ result = @test_throws ArgumentError parse(T, s, 16)
+ exception_with_base = result.value


I would find it clearer to always access result.value instead of defining exception_with_base (same below).

I defined them separately so that it reminds you they are exceptions, and so that in the test-failed message you see which one it was, since let-expressions obscure the line number.

Tetralux · 2017-02-13T12:27:43Z

So, the old parsing code for integers was forgiving of leading and trailing whitespace.
The old boolean parsing code was not.

My new integer parsing code is also forgiving of whitespace.
My new boolean parsing code is not.

Shall I make them both forgiving of it?

tkelman · 2017-02-14T04:31:43Z

either both should allow whitespace or neither should - maybe the latter if it's possible to get any performance benefits from that

Tetralux · 2017-02-14T04:38:49Z

@tkelman
With some naive benchmarking, it takes about ~~1ms~~ 2ms more time to be forgiving of whitespace in both cases.
It also seems that float parsing is also forgiving of whitespace.

Make parse display specific error messages if string is empty or co…

f363a85

…ntains only whitespace

Tetralux changed the title ~~Make parse(T, "") and parse(T, " ") etc, more specific~~ Make parse(T, "") and parse(T, " ") etc, more specific regarding whitespace and empty strings Feb 12, 2017

JeffBezanson requested changes Feb 12, 2017

View reviewed changes

Efficiency: do not use strip

d0eb2e3

Tetralux force-pushed the h5/parse_empty_string branch from 04035ab to d0eb2e3 Compare February 13, 2017 01:12

JeffBezanson reviewed Feb 13, 2017

View reviewed changes

Fix parsing of part of input string

3153af0

nalimilan reviewed Feb 13, 2017

View reviewed changes

Cleanup RE @nalimilan

5296502

Remove rogue space

81a71da

JeffBezanson approved these changes Feb 13, 2017

View reviewed changes

JeffBezanson merged commit 021e11e into JuliaLang:master Feb 13, 2017

Tetralux mentioned this pull request Feb 14, 2017

Be equally forgiving of leading & trailing whitespace in number parsing #20606

Merged

Tetralux deleted the h5/parse_empty_string branch February 14, 2017 05:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `parse(T, "")` and `parse(T, " ")` etc, more specific regarding whitespace and empty strings #20588

Make `parse(T, "")` and `parse(T, " ")` etc, more specific regarding whitespace and empty strings #20588

Tetralux commented Feb 12, 2017

JeffBezanson Feb 12, 2017

Tetralux Feb 12, 2017

Tetralux commented Feb 13, 2017

JeffBezanson Feb 13, 2017

Tetralux Feb 13, 2017

Tetralux commented Feb 13, 2017

tkelman commented Feb 13, 2017

nanosoldier commented Feb 13, 2017

nalimilan Feb 13, 2017

nalimilan Feb 13, 2017

nalimilan Feb 13, 2017

nalimilan Feb 13, 2017

nalimilan Feb 13, 2017

Tetralux Feb 13, 2017

nalimilan Feb 13, 2017

nalimilan Feb 13, 2017

nalimilan Feb 13, 2017

Tetralux Feb 13, 2017

Tetralux commented Feb 13, 2017

tkelman commented Feb 14, 2017

Tetralux commented Feb 14, 2017 •

edited

Loading

Make parse(T, "") and parse(T, " ") etc, more specific regarding whitespace and empty strings #20588

Make parse(T, "") and parse(T, " ") etc, more specific regarding whitespace and empty strings #20588

Conversation

Tetralux commented Feb 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tetralux commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tetralux commented Feb 13, 2017

tkelman commented Feb 13, 2017

nanosoldier commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tetralux commented Feb 13, 2017

tkelman commented Feb 14, 2017

Tetralux commented Feb 14, 2017 • edited Loading

Make `parse(T, "")` and `parse(T, " ")` etc, more specific regarding whitespace and empty strings #20588

Make `parse(T, "")` and `parse(T, " ")` etc, more specific regarding whitespace and empty strings #20588

Tetralux commented Feb 14, 2017 •

edited

Loading