-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fixes constructor of SubString from SubString #22511
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,7 +53,7 @@ function shell_parse(str::AbstractString, interpolate::Bool=true; | |
while !done(s,j) | ||
c, k = next(s,j) | ||
if !in_single_quotes && !in_double_quotes && isspace(c) | ||
update_arg(s[i:j-1]) | ||
update_arg(s[i:prevind(s, j)]) | ||
append_arg() | ||
j = k | ||
while !done(s,j) | ||
|
@@ -65,7 +65,7 @@ function shell_parse(str::AbstractString, interpolate::Bool=true; | |
j = k | ||
end | ||
elseif interpolate && !in_single_quotes && c == '$' | ||
update_arg(s[i:j-1]); i = k; j = k | ||
update_arg(s[i:prevind(s, j)]); i = k; j = k | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why these changes are being made as a part of this PR. What does this have to do with If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are two reasons:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @stevengj Is this justification OK? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I was thinking of |
||
if done(s,k) | ||
error("\$ right before end of command") | ||
end | ||
|
@@ -79,24 +79,24 @@ function shell_parse(str::AbstractString, interpolate::Bool=true; | |
else | ||
if !in_double_quotes && c == '\'' | ||
in_single_quotes = !in_single_quotes | ||
update_arg(s[i:j-1]); i = k | ||
update_arg(s[i:prevind(s, j)]); i = k | ||
elseif !in_single_quotes && c == '"' | ||
in_double_quotes = !in_double_quotes | ||
update_arg(s[i:j-1]); i = k | ||
update_arg(s[i:prevind(s, j)]); i = k | ||
elseif c == '\\' | ||
if in_double_quotes | ||
if done(s,k) | ||
error("unterminated double quote") | ||
end | ||
if s[k] == '"' || s[k] == '$' || s[k] == '\\' | ||
update_arg(s[i:j-1]); i = k | ||
update_arg(s[i:prevind(s, j)]); i = k | ||
c, k = next(s,k) | ||
end | ||
elseif !in_single_quotes | ||
if done(s,k) | ||
error("dangling backslash") | ||
end | ||
update_arg(s[i:j-1]); i = k | ||
update_arg(s[i:prevind(s, j)]); i = k | ||
c, k = next(s,k) | ||
end | ||
elseif !in_single_quotes && !in_double_quotes && c in special | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,32 +4,38 @@ | |
|
||
## substrings reference original strings ## | ||
|
||
""" | ||
SubString(s::AbstractString, i::Integer, j::Integer=endof(s)) | ||
SubString(s::AbstractString, r::UnitRange{<:Integer}) | ||
|
||
Like [`getindex`](@ref), but returns a view into the parent string `s` | ||
within range `i:j` or `r` respectively instead of making a copy. | ||
""" | ||
struct SubString{T<:AbstractString} <: AbstractString | ||
string::T | ||
offset::Int | ||
endof::Int | ||
|
||
function SubString{T}(s::T, i::Int, j::Int) where T<:AbstractString | ||
if i > endof(s) || j<i | ||
return new(s, i-1, 0) | ||
else | ||
if !isvalid(s,i) | ||
throw(ArgumentError("invalid SubString index")) | ||
end | ||
|
||
while !isvalid(s,j) && j > i | ||
j -= 1 | ||
end | ||
|
||
o = i-1 | ||
new(s, o, max(0, j-o)) | ||
end | ||
i > j && return new(s, i - 1, 0) # always allow i > j as it is consistent with getindex | ||
isvalid(s, i) || throw(BoundsError(s, i)) | ||
isvalid(s, j) || throw(BoundsError(s, j)) | ||
new(s, i-1, j-i+1) | ||
end | ||
end | ||
|
||
SubString(s::T, i::Int, j::Int) where {T<:AbstractString} = SubString{T}(s, i, j) | ||
SubString(s::SubString, i::Int, j::Int) = SubString(s.string, s.offset+i, s.offset+j) | ||
SubString(s::AbstractString, i::Integer, j::Integer) = SubString(s, Int(i), Int(j)) | ||
SubString(s::AbstractString, i::Integer) = SubString(s, i, endof(s)) | ||
SubString(s::AbstractString, i::Integer, j::Integer=endof(s)) = SubString(s, Int(i), Int(j)) | ||
SubString(s::AbstractString, r::UnitRange{<:Integer}) = SubString(s, first(r), last(r)) | ||
|
||
function SubString(s::SubString, i::Int, j::Int) | ||
# always allow i > j as it is consistent with getindex | ||
i > j && return SubString(s.string, s.offset + i, s.offset + j) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. first index: I pass I have to refer to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's converted to |
||
i >= 1 || throw(BoundsError(s, i)) | ||
j <= endof(s) || throw(BoundsError(s, j)) | ||
SubString(s.string, s.offset + i, s.offset + j) | ||
end | ||
|
||
SubString(s::AbstractString) = SubString(s, 1, endof(s)) | ||
SubString{T}(s::T) where {T<:AbstractString} = SubString{T}(s, 1, endof(s)) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,25 +25,53 @@ for i1 = 1:length(u8str2) | |
end | ||
end | ||
|
||
# tests that SubString of a single multibyte `Char` string, like "∀" which takes 3 bytes | ||
# gives the same result as `getindex` (except that it is a veiw not a copy) | ||
for idx in 0:1 | ||
@test SubString("∀", 1, idx) == "∀"[1:idx] | ||
end | ||
|
||
# Substring provided with invalid end index throws BoundsError | ||
@test_throws BoundsError SubString("∀", 1, 2) | ||
@test_throws BoundsError SubString("∀", 1, 3) | ||
@test_throws BoundsError SubString("∀", 1, 4) | ||
|
||
# Substring provided with invalid start index throws BoundsError | ||
@test_throws BoundsError SubString("∀∀", 2:4) | ||
|
||
# tests for SubString of more than one multibyte `Char` string | ||
# we are consistent with `getindex` for `String` | ||
for idx in [0, 1, 4] | ||
@test SubString("∀∀", 1, idx) == "∀∀"[1:idx] | ||
@test SubString("∀∀", 4, idx) == "∀∀"[4:idx] | ||
end | ||
|
||
# second index beyond endof("∀∀") | ||
for idx in 5:8 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be nice to have a comment explaining what is tested here. Same below, where some lines are commented and not others (a global comment for multiple lines is fine). |
||
@test_throws BoundsError SubString("∀∀", 1, idx) | ||
@test_throws BoundsError SubString("∀∀", 4, idx) | ||
end | ||
|
||
let str="tempus fugit" #length(str)==12 | ||
ss=SubString(str,1,length(str)) #match source string | ||
ss=SubString(str,1,endof(str)) #match source string | ||
@test length(ss)==length(str) | ||
|
||
ss=SubString(str,1:endof(str)) | ||
@test length(ss)==length(str) | ||
|
||
ss=SubString(str,1,0) #empty SubString | ||
@test length(ss)==0 | ||
|
||
ss=SubString(str,14,20) #start indexed beyond source string length | ||
ss=SubString(str,1:0) | ||
@test length(ss)==0 | ||
|
||
ss=SubString(str,10,16) #end indexed beyond source string length | ||
@test length(ss)==3 | ||
@test_throws BoundsError SubString(str,14,20) #start indexing beyond source string length | ||
@test_throws BoundsError SubString(str,10,16) #end indexing beyond source string length | ||
|
||
str2="" | ||
ss=SubString(str2,1,4) #empty source string | ||
@test length(ss)==0 | ||
|
||
ss=SubString(str2,1,1) #empty source string, identical start and end index | ||
@test length(ss)==0 | ||
@test_throws BoundsError SubString("", 1, 4) #empty source string | ||
@test_throws BoundsError SubString("", 1, 1) #empty source string, identical start and end index | ||
@test_throws BoundsError SubString("", 10, 12) | ||
@test SubString("",12,10) == "" | ||
end | ||
|
||
@test SubString("foobar", big(1), big(3)) == "foo" | ||
|
@@ -55,7 +83,7 @@ let str = "aa\u2200\u2222bb" | |
write(b, u) | ||
@test String(take!(b)) == "\u2200\u2222" | ||
|
||
@test_throws ArgumentError SubString(str, 4, 5) | ||
@test_throws BoundsError SubString(str, 4, 5) | ||
@test_throws BoundsError next(u, 0) | ||
@test_throws BoundsError next(u, 7) | ||
@test_throws BoundsError getindex(u, 0) | ||
|
@@ -68,21 +96,14 @@ let str = "aa\u2200\u2222bb" | |
end | ||
|
||
let str = "føøbar" | ||
@test_throws BoundsError SubString(str, 10, 10) | ||
u = SubString(str, 4, 3) | ||
@test length(u) == 0 | ||
b = IOBuffer() | ||
write(b, u) | ||
@test String(take!(b)) == "" | ||
end | ||
|
||
let str = "føøbar" | ||
u = SubString(str, 10, 10) | ||
@test length(u) == 0 | ||
b = IOBuffer() | ||
write(b, u) | ||
@test String(take!(b)) == "" | ||
end | ||
|
||
# search and SubString (issue #5679) | ||
let str = "Hello, world!" | ||
u = SubString(str, 1, 5) | ||
|
@@ -91,6 +112,22 @@ let str = "Hello, world!" | |
@test rsearch(u, "ll") == 3:4 | ||
end | ||
|
||
# SubString created from SubString | ||
let str = "Hello, world!" | ||
u = SubString(str, 2, 5) | ||
for idx in 1:4 | ||
@test SubString(u, 2, idx) == u[2:idx] | ||
@test SubString(u, 2:idx) == u[2:idx] | ||
end | ||
@test_throws BoundsError SubString(u, 1, 10) | ||
@test_throws BoundsError SubString(u, 1:10) | ||
@test_throws BoundsError SubString(u, 20:30) | ||
@test SubString(u, 20:15) == "" | ||
@test_throws BoundsError SubString(u, -1:10) | ||
@test SubString(u, -1, -10) == "" | ||
@test SubString(SubString("123", 1, 2), -10, -20) == "" | ||
end | ||
|
||
# sizeof | ||
@test sizeof(SubString("abc\u2222def",4,4)) == 3 | ||
|
||
|
@@ -115,8 +152,8 @@ let ss, s="lorem ipsum", | |
SubString(s,1,6)=>"lorem ", | ||
SubString(s,1,0)=>"", | ||
SubString(s,2,4)=>"ore", | ||
SubString(s,2,16)=>"orem ipsum", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe keep these examples, but with valid indices that still give the expected result? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
SubString(s,12,14)=>"" | ||
SubString(s,2,11)=>"orem ipsum", | ||
SubString(s,15,14)=>"" | ||
) | ||
for (ss,s) in sdict | ||
local ss | ||
|
@@ -140,9 +177,18 @@ end #let | |
|
||
#for isvalid(SubString{String}) | ||
let s = "Σx + βz - 2" | ||
for i in -1:(length(s) + 2) | ||
local ss = SubString(s, 1, i) | ||
@test isvalid(ss, i) == isvalid(s, i) | ||
for i in -1:(length(s)+2) | ||
if isvalid(s, i) | ||
ss=SubString(s,1,i) | ||
# make sure isvalid gives equivalent results for SubString and String | ||
@test isvalid(ss,i)==isvalid(s,i) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
else | ||
if i > 0 | ||
@test_throws BoundsError SubString(s,1,i) | ||
else | ||
@test SubString(s,1,i) == "" | ||
end | ||
end | ||
end | ||
end | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be listed under "breaking changes", since it can raise an error for previously working cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved, although if someone used an index outside a valid range it was probably a bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but that can still break code since no error was thrown before. Anyway, looks good to me now.