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

Eliminate use of undocumented Base.utf16_* functions (Fix #110) #111

Merged
merged 1 commit into from
Jul 1, 2015

Conversation

ScottPJones
Copy link
Contributor

This package used two undocumented, unexported functions from base/utf16.jl.
This removes that dependency, which broke things when those functions were renamed.

@stevengj
Copy link
Member

stevengj commented Jul 1, 2015

LGTM.

@stevengj
Copy link
Member

stevengj commented Jul 1, 2015

Test failure:

while loading C:\projects\json-jl\test\runtests.jl, in expression starting on line 193 
ERROR: LoadError: test failed: "휒" == "𝜒"
 in expression: obj["𝜒"] == "𝜒

@IainNZ
Copy link
Contributor

IainNZ commented Jul 1, 2015

Its perfectly possible those things that were in the <= 0.3.0- block were broken, as that code path hasn't been tested in 1 year+.

@stevengj
Copy link
Member

stevengj commented Jul 1, 2015

@ScottPJones, note that you should put (fixes #110) in the commit message, so that the corresponding issue is automatically closed (and cross-referenced) when this is merged. Also, a more specific commit message would be nice, e.g. eliminate use of undocumented Base.utf16_* functions (fixes #110).

@ScottPJones
Copy link
Contributor Author

Yes, I know that, just rushing to get this fixed, sorry!

@ScottPJones ScottPJones changed the title Fix use of unexported functions from Base Eliminate use of undocumented Base.utf16_* functions (Fix #110) Jul 1, 2015
@ScottPJones
Copy link
Contributor Author

@IainNZ You were correct, I shouldn't have relied on those old functions (they didn't cast to UInt32 before doing the << 10, and lost the top bits). They looked almost identical to what was in utf16.jl,
but looks can be deceiving!

@IainNZ
Copy link
Contributor

IainNZ commented Jul 1, 2015

Heh, yeah. I figured your Unicode-foo would know what the right thing to do was, hopefully Travis agrees!

@ScottPJones
Copy link
Contributor Author

Now, if somebody can merge this, so I don't annoy people trying to use JSON 😢!

@IainNZ
Copy link
Contributor

IainNZ commented Jul 1, 2015

Waiting for CI to go green

@ScottPJones
Copy link
Contributor Author

Smart man! Thinking about it, I wonder if the old code just bit rotted. Did unsigned values used to end up as UInt, like signed values? I tripped over when I started in Julia because of the fact that unsigned values go from UInt8, UInt16, UInt32 instead of UInt)

IainNZ added a commit that referenced this pull request Jul 1, 2015
Eliminate use of undocumented Base.utf16_* functions (Fix #110)
@IainNZ IainNZ merged commit 0b6e9ec into JuliaIO:master Jul 1, 2015
@IainNZ
Copy link
Contributor

IainNZ commented Jul 1, 2015

Not sure

@IainNZ
Copy link
Contributor

IainNZ commented Jul 1, 2015

Tagging now

@stevengj
Copy link
Member

stevengj commented Jul 2, 2015

Yes, in old versions of Julia, integer arithmetic promoted to Int/UInt.

@ScottPJones ScottPJones deleted the spj/utf16 branch July 2, 2015 14:24
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.

None yet

3 participants