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.

Sign up for GitHub

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

Jump to bottom

correct nextind/prevind/thisind for SubString{String} #25531

Merged
merged 2 commits into from
Feb 4, 2018
Merged

correct nextind/prevind/thisind for SubString{String} #25531

merged 2 commits into from
Feb 4, 2018

Conversation

bkamins
Copy link
Member

@bkamins bkamins commented Jan 12, 2018

An attempt to correct incorrect behavior reported in #25478.

Also fixes those two problems:

julia> s = "∀∃∀"
"∀∃∀"

julia> ss = SubString(s, 4, 4)
"∃"

julia> thisind(ss, 0)
-2

julia> prevind(ss, 1)
-2

In general I want SubString to behave identically to String created using getindex with the same range.

It seems that it is enough to remove all methods from substring.jl and extend signatures in string.jl, but I am still checking corner cases, so it is WIP still (but hopefully CI will pass).

@ararslan ararslan added the strings "Strings!" label Jan 12, 2018
StefanKarpinski
StefanKarpinski reviewed Jan 13, 2018
View reviewed changes
test/strings/types.jl Outdated
@@ -188,6 +188,25 @@ let s = "lorem ipsum", sdict = Dict(
end
end

# proper nextind/prevind/thisind for SubString{String}
let s = "∀∃∀", ss = SubString(s, 4, 4), s2 = s[4:4]
Copy link
Member

Choose a reason for hiding this comment

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

Is this meant to be just one character for the test only? Maybe test more substrings?

@bkamins bkamins force-pushed the SubString_nextind branch 3 times, most recently from a3c7eac to 5912d26 Compare January 13, 2018 15:05
@bkamins
Copy link
Member Author

bkamins commented Jan 13, 2018

I have to wait with this PR till #25533 is merged because of an error in implementation of thisind. Cascading bug fixing unfortunately.

@bkamins bkamins mentioned this pull request Jan 13, 2018
Merged
@bkamins
Copy link
Member Author

bkamins commented Jan 16, 2018

Another bug to test this PR against:

julia> x = SubString("123", 20, 19)
""

julia> thisind(x, 0)
ERROR: BoundsError: attempt to access "123"
  at index [19]
Stacktrace:
 [1] thisind(::String, ::Int64) at .\strings\string.jl:104
 [2] thisind(::SubString{String}, ::Int64) at .\strings\substring.jl:83
 [3] top-level scope

@bkamins bkamins mentioned this pull request Jan 16, 2018
Open
@bkamins bkamins changed the title WIP: correct nextind/prevind/thisind for SubString{String} correct nextind/prevind/thisind for SubString{String} Jan 27, 2018
@bkamins
Copy link
Member Author

bkamins commented Jan 28, 2018

The CI errors seem unrelated so this PR should be good for review and merge.

@StefanKarpinski StefanKarpinski self-assigned this Jan 28, 2018
nalimilan
nalimilan reviewed Jan 29, 2018
View reviewed changes
base/strings/string.jl Outdated
function nextind(s::String, i::Int)
nextind(s::String, i::Int) = _nextind_str(s, i)

function _nextind_str(s, i::Int)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add ::AbstractString?

Copy link
Member Author

Choose a reason for hiding this comment

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

An optimal option would be to define nextind(s::Union{String, SubString{String}}, i::Int) (the same for thisind), but then I would have to move it to other place in the code after SubString is defined. What do you think?

Adding ::AbstractSting is misleading as _nextind_str will not work for any AbstractString and it is intended for internal use only.

So I would be obliged for a guidance what is the optimal design of code for such cases (i.e. the same body of a method applies to two different types, but at the moment of definition of the method only one of those types is defined)?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say it's fine as it is given that bootstrapping is difficult. Maybe just add a comment saying that s::Union{String, SubString{String}}.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added - I hope this is what you meant.

@bkamins
Copy link
Member Author

bkamins commented Feb 3, 2018

Is this OK to be merged?

@StefanKarpinski
Copy link
Member

Rerunning the tests to be sure, but then yes, this is good to go.

@bkamins
Copy link
Member Author

bkamins commented Feb 4, 2018

✔️ flush.

@StefanKarpinski StefanKarpinski merged commit 80abd3d into JuliaLang:master Feb 4, 2018
@bkamins bkamins deleted the SubString_nextind branch February 4, 2018 09:52
@bkamins bkamins mentioned this pull request Feb 4, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nalimilan nalimilan nalimilan left review comments

@StefanKarpinski StefanKarpinski StefanKarpinski approved these changes

Assignees

@StefanKarpinski StefanKarpinski

Labels
strings "Strings!"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bkamins @StefanKarpinski @nalimilan @ararslan