WIP: Remove RevString #23612

ararslan · 2017-09-07T02:55:36Z

This PR removes RevString from Base. It's the companion to JuliaStrings/LegacyStrings.jl#22, which adds RevString to LegacyStrings.

Fixes #22611.

ararslan · 2017-09-07T02:57:16Z

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

nanosoldier · 2017-09-07T04:25:09Z

Your benchmark job has completed, but no benchmarks were actually executed. Perhaps your tag predicate contains misspelled tags? cc @ararslan

ararslan · 2017-09-07T05:04:38Z

Whoops. @nanosoldier runbenchmarks("string", vs=":master")

nanosoldier · 2017-09-07T06:33:27Z

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

fredrikekre · 2017-09-07T10:50:10Z

base/strings/types.jl

@@ -99,32 +99,18 @@ end

 ## reversed strings without data movement ##


This comment should be removed as well?

nalimilan

Thanks! It's hard to check that all computations of indices are correct, but this looks good and tests pass...

nalimilan · 2017-09-07T12:25:58Z

base/repl/REPLCompletions.jl

@@ -239,18 +238,19 @@ function find_start_brace(s::AbstractString; c_start='(', c_end=')')
                in_back_ticks = true
            end
        else
-            if !in_back_ticks && !in_double_quotes && c == '\'' && !done(r, i) && next(r, i)[1]!='\\'
+            if !in_back_ticks && !in_double_quotes && c == '\'' && i > 0 && s[prevind(s, i)] != '\\'


It would make sense to save the result of prevind(s, i) since it's also used below.

nalimilan · 2017-09-07T12:31:01Z

base/strings/search.jl

-    e-j+1
+function rsearch(s::AbstractString, c::Chars, i::Integer=start(s))
+    if isempty(c)
+        return 1 <= i <= nextind(s, endof(s)) ? i : throw(BoundsError(s, i))


I don't think indices beyond endof(s) should be allowed. See also #22548 and #22572. You could probably use checkbounds for that? Also, it looks like you could first check the bounds using the block below, and only then handle the special case of empty c?

endof(s) is the last valid index, so 1 <= i <= endof(s) should be sufficient.

Sounds good. I was just mimicking the structure of the search code here.

Ah, OK. So maybe better keep the same (weird) approach as search and fix that later.

Seems like I might as well go with the better approach now and update search to match.

nalimilan · 2017-09-07T12:33:27Z

base/strings/search.jl

 end

 function _rsearchindex(s, t, i)
    if isempty(t)
        return 1 <= i <= nextind(s,endof(s)) ? i :
               throw(BoundsError(s, i))
    end
-    t = RevString(t)
-    rs = RevString(s)
+    t = reverse(t)


Why not implement the reversed algorithm just like above?

Wasn't sure how best to do it. Suggestions welcome.

I don't know... Doesn't changing every next into prevind work like you did above? I admit that method looks a bit scary. Or just keep RevString as an internal helper type just for that method. ;-)

nalimilan · 2017-09-07T12:34:34Z

base/strings/types.jl

@@ -1,6 +1,6 @@
 # This file is a part of Julia. License is MIT: https://julialang.org/license

-# SubString and RevString types
+# SubString type


This file is kind of inconsistent now. Maybe better rename it to substring.jl and move unrelated functions elsewhere?

Sounds good. Once I'm done with implementations and cleanup here I'll make that a separate commit.

nalimilan · 2017-09-07T12:34:44Z

base/strings/types.jl

@@ -99,32 +99,18 @@ end

 ## reversed strings without data movement ##


"without data movement"?

nalimilan · 2017-09-07T12:35:41Z

contrib/julia.xml

@@ -158,7 +158,6 @@
      <item> RegexMatch </item>
      <item> RegexMatchIterator </item>
      <item> RepString </item>


Could remove RepString too while you're at it (probably in other files too; but are they maintained?).

Ah, that must have been an oversight in #19867. I don't even know what this file is for.

nalimilan · 2017-09-07T12:37:02Z

test/strings/types.jl

@@ -174,11 +165,6 @@ for T in (String, GenericString)
                s = convert(T, string(prefix, c, suffix))
                r = reverse(s)
                ri = search(r, c)
-                @test r == RevString(s)


Hmm, you don't check that r is correct now. Should probably hardcode the results.

We have more extensive tests for the correctness of reverse in test/strings/unicode.jl; I assume those would be sufficient.

ararslan · 2017-09-08T20:04:27Z

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

nanosoldier · 2017-09-08T20:57:49Z

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

ararslan · 2017-09-08T22:55:40Z

To remove RevString we need a type for the general reverse(::AbstractString) case, and the most natural choice is String. However, there is a difference in indexing behavior, which is proving problematic:

julia> s = "X\U0001d4c1β\U0001d6a4\U0001d4c1β\U0001d6a4"
"X𝓁β𝚤𝓁β𝚤"

julia> b = SubString(s, 1, 8)
"X𝓁β𝚤"

julia> r = reverse(b)
"𝚤β𝓁X"

julia> search(r, 'X')
8

julia> search(String(r), 'X')
11

These results are both valid indices for their inputs and indeed they correspond to the same character (i.e. r[8] == String(r)[11] == 'X'). However, the difference in indexing implies that s[reverseind(s, i)] isn't necessarily valid, nor is it necessarily the same as reverse(s)[i]. This is causing the tests to fail. (And in fact, marking them as broken also causes the tests to fail, since in some cases it does work as expected.)

ararslan · 2017-09-10T19:16:25Z

Bump. Any ideas about the indexing? It would also be good to hear from @stevengj, who introduced reverseind IIRC.

nalimilan · 2017-09-10T19:44:05Z

Just to restate my position: let's get rid of reversind, which is used in only one place in Base.

ararslan · 2017-09-12T17:45:32Z

I'd be okay keeping reverseind if there's a potential resolution to the indexing issue, e.g. deprecating reverseind(s::AbstractString, i) in favor of reverseind(convert(String, s), i) or something so that the indexing matches, or revamping the implementation of reverseind so that it's aware that the input may have different indexing than reverse.

nalimilan · 2017-09-14T16:32:14Z

BTW, I've noticed the following bug on current master:

julia> rsearch(SubString("", 1, 0), "")
ERROR: BoundsError: attempt to access ""
  at index [0]
Stacktrace:
 [1] _rsearchindex(::SubString{String}, ::String, ::Int64) at ./strings/search.jl:219
 [2] _rsearch(::SubString{String}, ::String, ::Int64) at ./strings/search.jl:357
 [3] rsearch(::SubString{String}, ::String) at ./strings/search.jl:365
 [4] macro expansion at ./REPL.jl:97 [inlined]
 [5] (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:73

stevengj · 2017-09-19T16:37:00Z

The basic motivation for reverseind (from #9249) hasn't changed: the only reason we have Base.reverse is to support reversed regex searching and other reversed processing by external libraries (#6165). As long as we export reverse for such uses, we need to export reverseind as otherwise the index mapping is extremely error prone for UTF-8.

ararslan · 2017-09-19T17:30:57Z

That's fine. What do you propose I do in this PR then, given the difference in indexing?

stevengj · 2017-09-19T17:37:51Z

Remove reverse(::AbstractString)?

ararslan · 2017-09-19T17:44:52Z

Yeah, I guess that would be okay. It'd be a pretty breaking change without providing a deprecation though. (Though I suppose not all that much more breaking than silently changing the behavior to convert to String.) Any idea what a sensible deprecation would look like?

stevengj · 2017-09-19T17:48:53Z

Deprecate to reverse(String(s)) so that the conversion is explicit

ararslan · 2017-09-21T04:13:57Z

Hm, I just noticed that in deprecating RepString, we didn't deprecate repeat(::AbstractString) but instead defined it to convert to String first. Should we be consistent with reverse and do the conversion (breaking change) rather than the deprecation?

nalimilan · 2017-09-21T08:50:22Z

base/strings/search.jl

                matched = false
                break
            end
-            c, k = next(rs,k)
-            d, j = next(t,j)
+            # Using `reverseind` with `prevind` mimics `next` but for iteration over


I still wonder whether it wouldn't be more efficient to work with indices in the original string (using prevind), and only call reverseind once the match is found. Or maybe I'm missing something?

Probably, though this approach didn't introduce any regressions as far as Nanosoldier could tell. My problem with changing it is that it took me quite a while to figure out how to even get to this; translating this algorithm to be in reverse broke my brain a bit. I'd be happy to change it with some guidance though.

Yeah maybe just leave a TODO so that we can find it when looking for possible performance enhancements.

If you want to try changing it, I'd say you should just have to call prevind on both rs and t, extract the chars and compare them as currently? Actually I don't understand why reversind is needed here, nor how it could be correct. In the first iteration, k contains the index in s, not in reverse(s), so how can that work?

I wish I knew.

nalimilan · 2017-09-21T08:53:39Z

Hm, I just noticed that in deprecating RepString, we didn't deprecate repeat(::AbstractString) but instead defined it to convert to String first. Should we be consistent with reverse and do the conversion (breaking change) rather than the deprecation?

Yeah, why not. Converting to String is a reasonable fallback, and anyway by calling reverse you accepted (after this PR) the allocation of a string of the same length already.

ararslan · 2017-10-07T07:21:37Z

I think things should be all sorted out now, if folks want to have another look.

nalimilan · 2017-10-07T12:15:10Z

This would look good... except that CI doesn't pass. :-p

ararslan · 2017-10-07T19:25:29Z

Okay, well that's what I get for only running a subset of the tests locally. 😑

stevengj · 2017-10-23T18:09:40Z

NEWS.md

@@ -243,6 +246,9 @@ This section lists changes that do not have deprecation warnings.
  * All command line arguments passed via `-e`, `-E`, and `-L` will be executed in the order
    given on the command line ([#23665]).

+  * `reverse(::AbstractString)` now unconditionally returns a `String`. Previously it
+    returned a `RepString`, which has been removed from Base ([#23612]).


RepString -> RevString

The `RevString` type for lazily reversed strings has been moved to the LegacyStrings package. Fixes #22611. Calling `reverse` on an `AbstractString` with no more specific method now unconditionally returns a `String`.

Move the non-substring related functionality to strings/basic.jl.

StefanKarpinski · 2017-11-22T15:55:09Z

Hey @ararslan, just so you know, I'm about to push a branch that deletes RevString; it was surprisingly involved and involved changing a generic reverseind, which in turn required adding ncodeunits and thisind. Sorry for the effort you put into this :(

These seem unrelated, but they're actually linked: * If you reverse generic strings by wrapping them in `RevString` then then this generic `reverseind` is incorrect. * In order to have a correct generic `reverseind` one needs to assume that `reverse(s)` returns a string of the same type and encoding as `s` with code points in reverse order; one also needs to assume that the code units encoding each character remain the same when reversed. This is a valid assumption for UTF-8, UTF-16 and (trivially) UTF-32. Reverse string search functions are pretty messed up by this and I've fixed them well enough to work but they may be quite inefficient for long strings now. I'm not going to spend too much time on this since there's other work going on to generalize and unify searching APIs. Close #22611 Close #24613 See also: #10593 #23612 #24103

These seem unrelated, but they're actually linked: * If you reverse generic strings by wrapping them in `RevString` then then this generic `reverseind` is incorrect. * In order to have a correct generic `reverseind` one needs to assume that `reverse(s)` returns a string of the same type and encoding as `s` with code points in reverse order; one also needs to assume that the code units encoding each character remain the same when reversed. This is a valid assumption for UTF-8, UTF-16 and (trivially) UTF-32. Reverse string search functions are pretty messed up by this and I've fixed them well enough to work but they may be quite inefficient for long strings now. I'm not going to spend too much time on this since there's other work going on to generalize and unify searching APIs. Close JuliaLang#22611 Close JuliaLang#24613 See also: JuliaLang#10593 JuliaLang#23612 JuliaLang#24103

ararslan added deprecation This change introduces or involves a deprecation strings "Strings!" labels Sep 7, 2017

ararslan requested review from StefanKarpinski, JeffBezanson and nalimilan September 7, 2017 02:55

ararslan added this to the 1.0 milestone Sep 7, 2017

ararslan force-pushed the aa/no-revstring branch from f9cad23 to 18d1ef9 Compare September 7, 2017 04:58

ararslan changed the title ~~Remove RevString~~ WIP: Remove RevString Sep 7, 2017

fredrikekre reviewed Sep 7, 2017

View reviewed changes

nalimilan reviewed Sep 7, 2017

View reviewed changes

ararslan force-pushed the aa/no-revstring branch from 18d1ef9 to a33730d Compare September 8, 2017 16:59

ararslan force-pushed the aa/no-revstring branch 2 times, most recently from ff9744e to 679e7ee Compare September 10, 2017 23:42

ararslan requested a review from stevengj September 13, 2017 20:23

ararslan force-pushed the aa/no-revstring branch from ec7f783 to ebc668d Compare September 21, 2017 04:14

nalimilan reviewed Sep 21, 2017

View reviewed changes

ararslan force-pushed the aa/no-revstring branch from ebc668d to 9debcf0 Compare September 21, 2017 16:58

ararslan added the breaking This change will break code label Sep 21, 2017

ararslan added the excision Removal of code from Base or the repository label Sep 29, 2017

ararslan force-pushed the aa/no-revstring branch from 9debcf0 to 3e06ccd Compare October 7, 2017 06:44

ararslan changed the title ~~WIP: Remove RevString~~ Remove RevString Oct 7, 2017

ararslan changed the title ~~Remove RevString~~ WIP: Remove RevString Oct 7, 2017

stevengj reviewed Oct 23, 2017

View reviewed changes

stevengj mentioned this pull request Nov 20, 2017

make reverseind generic #24613

Closed

ararslan added 2 commits November 21, 2017 23:47

Remove RevString

0e7b347

The `RevString` type for lazily reversed strings has been moved to the LegacyStrings package. Fixes #22611. Calling `reverse` on an `AbstractString` with no more specific method now unconditionally returns a `String`.

Rename strings/types.jl to strings/substring.jl

1693b04

Move the non-substring related functionality to strings/basic.jl.

ararslan force-pushed the aa/no-revstring branch from 3e06ccd to 1693b04 Compare November 22, 2017 07:47

StefanKarpinski mentioned this pull request Nov 22, 2017

remove RevString; efficient generic reverseind #24708

Merged

ararslan closed this Nov 22, 2017

ararslan deleted the aa/no-revstring branch November 22, 2017 19:46

		@@ -99,32 +99,18 @@ end

		## reversed strings without data movement ##

WIP: Remove RevString #23612

WIP: Remove RevString #23612

Conversation

ararslan commented Sep 7, 2017

ararslan commented Sep 7, 2017

nanosoldier commented Sep 7, 2017

ararslan commented Sep 7, 2017

nanosoldier commented Sep 7, 2017

Choose a reason for hiding this comment

nalimilan left a 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

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

ararslan commented Sep 8, 2017

nanosoldier commented Sep 8, 2017

ararslan commented Sep 8, 2017

ararslan commented Sep 10, 2017

nalimilan commented Sep 10, 2017

ararslan commented Sep 12, 2017

nalimilan commented Sep 14, 2017

stevengj commented Sep 19, 2017

ararslan commented Sep 19, 2017

stevengj commented Sep 19, 2017 • edited Loading

ararslan commented Sep 19, 2017

stevengj commented Sep 19, 2017

ararslan commented Sep 21, 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

nalimilan commented Sep 21, 2017

ararslan commented Oct 7, 2017

nalimilan commented Oct 7, 2017

ararslan commented Oct 7, 2017

Choose a reason for hiding this comment

StefanKarpinski commented Nov 22, 2017

stevengj commented Sep 19, 2017 •

edited

Loading