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

Fix test failure with ReinterpretArray #25

Merged
merged 1 commit into from
Oct 24, 2017
Merged

Fix test failure with ReinterpretArray #25

merged 1 commit into from
Oct 24, 2017

Conversation

nalimilan
Copy link
Member

@nalimilan nalimilan commented Oct 22, 2017

next(x, endof(x)) is not generally valid with AbstractVectors even though
it works for Vector. This fixes a test failure on Julia 0.7.

The UTF32String(::Vector{Char}) constructor added a \0 char at the end of the string because it used reinterpret(UInt32, ...), which did not dispatch to the inner constructor directly as it used to do on 0.6.

@nalimilan
Copy link
Member Author

Hmm, there's still another error to fix on 0.7...

@ararslan
Copy link
Member

Looks like something is or is not null terminated only on 0.7???

next(x, endof(x)) is not generally valid with AbstractVectors even though
it works for Vector. This fixes a test failure on Julia 0.7.

The UTF32String(::Vector{Char}) constructor added a \0 char at the end
of the string because it used reinterpret(UInt32, ...), which did not
dispatch to the inner constructor directly as it used to do on 0.6.
@nalimilan
Copy link
Member Author

OK I've found the problem. reinterpret was used and was supposed to return a Vector so that the call was dispatched to the inner constructor, but with ReinterpretArray it didn't work. These constructors are pretty weird anyway, since whether a '\0' is added depends on whether you pass a Vector{UInt32} or another AbstractVector{UInt32}...

@ararslan
Copy link
Member

Good catch!

@nalimilan nalimilan merged commit 873a1b4 into master Oct 24, 2017
@nalimilan nalimilan deleted the nl/vector branch October 24, 2017 20:28
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.

2 participants