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.

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

move DirectIndexString from Base #24

Merged
merged 2 commits into from
Nov 19, 2017
Merged

Conversation

bkamins
Copy link
Contributor

@bkamins bkamins commented Oct 21, 2017

PR following JuliaLang/julia#24259

@@ -97,4 +98,11 @@ using Compat
else
include("rep.jl")
end

if isdefined(Base, :DirectIndexString)
Copy link
Member

Choose a reason for hiding this comment

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

I think you'll have to define this earlier, since DirectIndexString is used above. You'll be able to check that once you'll have a version of Julia without it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved

Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

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

Looks good to me, the code is tested via ASCIIString. Though we should probably wait until JuliaLang/julia#24259 has been merged to check that tests pass without DirectIndexString in Base.

EDIT: the problem is, tests currently fail on 0.7...

@ararslan
Copy link
Member

Looks good to me as well. I assume the UTF16 BoundsError is preexisting.

@bkamins
Copy link
Contributor Author

bkamins commented Oct 21, 2017

Yes - the error seems to be line 763 of abstractarray.jl which is called with a number.

@ararslan
Copy link
Member

Are there tests for DirectIndexString in Base that could be copied over?

@bkamins
Copy link
Contributor Author

bkamins commented Oct 22, 2017

I have moved everything from Base that referenced DirectIndexString.
No tests used DirectIndexString as no type in Base was subtype of DirectIndexString.

This part of code:

# isvalid(), chr2ind() and ind2chr() for SubString{DirectIndexString}
let ss, s="lorem ipsum",
    sdict=Dict(SubString(s,1,11)=>s,
               SubString(s,1,6)=>"lorem ",
               SubString(s,1,0)=>"",
               SubString(s,2,4)=>"ore",
               SubString(s,2,11)=>"orem ipsum",
               SubString(s,15,14)=>""
               )
    for (ss,s) in sdict
        local ss
        for i in -1:12
            @test isvalid(ss,i)==isvalid(s,i)
        end
    end
    for (ss,s) in sdict
        local ss
        for i in 1:length(ss)
            @test ind2chr(ss,i)==ind2chr(s,i)
        end
    end
    for (ss,s) in sdict
        local ss
        for i in 1:length(ss)
            @test chr2ind(ss,i)==chr2ind(s,i)
        end
    end
end #let

seems to have been testing DirectIndexString in the past but now it tests String containing only ASCII (I have corrected the description in JuliaLang/julia#24262).

@ararslan
Copy link
Member

We should try to get this merged and tagged ASAP to avoid a gap in availability on 0.7. Any ideas about the 0.7 failures, @nalimilan?

@nalimilan
Copy link
Member

Honestly I don't think anybody cares much about DirectIndexString... :-)

I've traced the failure to ReinterpretArray, see JuliaLang/julia#23750 (comment). The problematic call is convert(UTF16String, Int16[[0x65, 0x66] [0x67, 0x68]]), which I really wonder how somebody thought it was worth supporting... The next level of absurdity will be to allow constructing strings from BitArray{3}.

@nalimilan
Copy link
Member

I have a fix at #25.

@nalimilan nalimilan closed this Oct 24, 2017
@nalimilan nalimilan reopened this Oct 24, 2017
@stevengj
Copy link
Member

stevengj commented Nov 15, 2017

ASCIIString and UTF32String are both subtypes of DirectIndexString and get methods like nextind and length from DirectIndexString. So, the tests for ASCIIString and UTF32String are implicitly testing DirectIndexString. And people do care about those types.

@nalimilan nalimilan merged commit c19101a into JuliaStrings:master Nov 19, 2017
@bkamins bkamins deleted the DIS branch November 19, 2017 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants