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

Add Compat.StringVector #348

Merged
merged 7 commits into from
Apr 19, 2017
Merged

Add Compat.StringVector #348

merged 7 commits into from
Apr 19, 2017

Conversation

TotalVerb
Copy link
Contributor

This is just Vector{UInt8} on 0.5 and below, and Base.StringVector on 0.6.

@stevengj
Copy link
Member

See also JuliaLang/julia#19945 ... it would be nice to decide whether we're actually going to document/export this in Base.

@TotalVerb
Copy link
Contributor Author

I think we should have this regardless of whether it will be documented/exported in Base. We had promote_eltype_op in Compat, after all. If the API changes, we can deprecate it as we've done with promote_eltype_op here.

@TotalVerb
Copy link
Contributor Author

TotalVerb commented Apr 13, 2017

The 0.4 test is failing because String(x) calls bytestring(x) which does not properly alias array x, instead making a copy. We should have String(x) call UTF8String(x) instead, which is the 0.4 equivalent of String. There don't seem to be any downsides to always returning UTF8String instead of sometimes ASCIIString and sometimes UTF8String. Any objections to making this change?

@tkelman
Copy link
Contributor

tkelman commented Apr 13, 2017

changing the return type like that seems risky. could this instead wait until Compat drops 0.4 support?

@TotalVerb
Copy link
Contributor Author

We can also disable that particular test on all versions of 0.4 that have String(x::Vector{UInt8}) call bytestring, which should be fine.

README.md Outdated
@@ -163,6 +163,8 @@ Currently, the `@compat` macro supports the following syntaxes:

* `bswap` is supported for `Complex` arguments on 0.5 and below. ([#21346])

* `Compat.StringVector` is supported on 0.5 and below. ([#19449])
Copy link
Member

@stevengj stevengj Apr 14, 2017

Choose a reason for hiding this comment

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

Should mention that it is an alias for Base.StringVector on 0.6

src/Compat.jl Outdated
@@ -1464,6 +1464,13 @@ if VERSION < v"0.6.0-pre.beta.102"
Base.bswap(z::Complex) = Complex(bswap(real(z)), bswap(imag(z)))
end

# https://github.com/JuliaLang/julia/pull/19449
if VERSION < v"0.6.0-dev.1988"
StringVector = Vector{UInt8}
Copy link
Member

@stevengj stevengj Apr 14, 2017

Choose a reason for hiding this comment

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

should probably be

StringVector(n::Integer) = Vector{UInt8}(n)

since doing StringVector = Vector{UInt8} has the drawbacks that (a) it is not const and (b) it accepts multiple arguments, unlike Base.StringVector in 0.6.

@TotalVerb
Copy link
Contributor Author

Thanks, I addressed the comments.

README.md Outdated
@@ -163,6 +163,8 @@ Currently, the `@compat` macro supports the following syntaxes:

* `bswap` is supported for `Complex` arguments on 0.5 and below. ([#21346])

* `Compat.StringVector` is supported on 0.5 and below. On 0.6 and later, it aliases `Base.StringVector`. ([#19449])
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it should suggest using Compat.UTF8String(Compat.StringVector(n)) to create strings from arrays without making copies? This way the code would work on 0.4 too. The test could be modified accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0.4 is on its way out, and many users might not know/care about Compat.UTF8String. But you are right about the test. I'll change that.

Copy link
Member

Choose a reason for hiding this comment

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

Until Compat drops support for 0.4, we should explain how to use it "on 0.5 and below".

Copy link
Contributor Author

@TotalVerb TotalVerb Apr 15, 2017

Choose a reason for hiding this comment

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

I've updated the README. How's this?

@TotalVerb
Copy link
Contributor Author

The Mac failure is concerning. Does anyone have an idea what's going on?

@TotalVerb
Copy link
Contributor Author

TotalVerb commented Apr 15, 2017

Although the Travis failure seems to have disappeared overnight, the segfault is still concerning. Should I raise an issue in Base? It may be hard to reproduce.

@TotalVerb
Copy link
Contributor Author

@stevengj Do you think the current version is good to go?

@stevengj stevengj merged commit 49e0235 into master Apr 19, 2017
@stevengj stevengj deleted the fw/stringvector branch April 19, 2017 02:16
martinholters added a commit that referenced this pull request Sep 10, 2018
fredrikekre pushed a commit that referenced this pull request Sep 14, 2018
* `zeros` and `ones` with interface of `similar` (from #330)

* `convert` between `Set` types (from #342)

* `isassigned(::RefValue)` (from #345)

* `unsafe_trunc(::Type{<:Integer}, ::Integer)` (from #344)

* `bswap` for complex numbers (from #346)

* Compat.StringVector (from #348)

* `invokelatest` (from #352 and #359)

* Misc. pre-0.6-only code

* obsolete README enries
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.

3 participants