Skip to content

Commit

Permalink
type-stability for lstrip, rstrip, strip
Browse files Browse the repository at this point in the history
  • Loading branch information
StefanKarpinski committed Aug 21, 2015
1 parent ff1b30b commit 7676a6a
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 2 deletions.
4 changes: 2 additions & 2 deletions base/strings/util.jl
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ function lstrip(s::AbstractString, chars::Chars=_default_delims)
end
i = j
end
""
s[end+1:end]

This comment has been minimized.

Copy link
@ScottPJones

ScottPJones Aug 22, 2015

Contributor

Why is this end+1:end instead of 1:0, as in the next one?
Potentially costly?

This comment has been minimized.

Copy link
@StefanKarpinski

StefanKarpinski Aug 22, 2015

Author Member

Conceptually it's an empty string at the end of the input string. May or may not matter to the result (for substrings, there's an observable difference), but it seems more consistent.

This comment has been minimized.

Copy link
@ScottPJones

ScottPJones Aug 22, 2015

Contributor

Conceptually it's an empty string at the end of the input string

I already had understood that.

What is the difference for substrings?
I'd be worried about performance penalties. You can explain the conceptual bit in a comment, doesn't need to slow down the code used by many people, to make a conceptual point in something that not many people would ever read.

This comment has been minimized.

Copy link
@ScottPJones

ScottPJones Aug 22, 2015

Contributor

I think changing things to be type stable here is very good, 👍, just want to make sure no unnecessary overhead in new code.

end

function rstrip(s::AbstractString, chars::Chars=_default_delims)
Expand All @@ -83,7 +83,7 @@ function rstrip(s::AbstractString, chars::Chars=_default_delims)
end
i = j
end
""
s[1:0]
end

strip(s::AbstractString) = lstrip(rstrip(s))
Expand Down
21 changes: 21 additions & 0 deletions test/strings/util.jl
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,30 @@
@test rpad("foo", 6, " ") == "foo "

# string manipulation
@test strip("") == ""
@test strip(" ") == ""
@test strip(" ") == ""
@test strip(" ") == ""
@test strip("\t hi \n") == "hi"
@test strip("foobarfoo", ['f', 'o']) == "bar"

for s in ("", " ", " abc", "abc ", " abc "), f in (lstrip, rstrip, strip)
fs = f(s)

This comment has been minimized.

Copy link
@tkelman

tkelman Aug 22, 2015

Contributor

boo, tabs

This comment has been minimized.

Copy link
@StefanKarpinski

StefanKarpinski Aug 22, 2015

Author Member

sorry about that, for some reason Sublime Text is kind of obstinate about tabs

This comment has been minimized.

Copy link
@tkelman

tkelman Aug 22, 2015

Contributor

Odd, I've found it to be the opposite, and unusually hard to insert a tab if you actually need one, except when editing a makefile. Must have it configured differently.

This comment has been minimized.

Copy link
@StefanKarpinski

StefanKarpinski Aug 22, 2015

Author Member

I need to figure out that configuration :-\

for T = (ASCIIString, UTF8String, UTF16String, UTF32String)
t = convert(T,s)
ft = f(t)
@test s == t
@test fs == ft
@test typeof(ft) == typeof(t[1:end])

b = convert(SubString{T}, t)
fb = f(b)
@test s == b
@test fs == fb
@test typeof(fb) == SubString{T}
end
end

# split
@test isequal(split("foo,bar,baz", 'x'), ["foo,bar,baz"])
@test isequal(split("foo,bar,baz", ','), ["foo","bar","baz"])
Expand Down

0 comments on commit 7676a6a

Please sign in to comment.