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.

Sign up for GitHub

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

Jump to bottom

replace bytestring with String #16453

Merged
merged 1 commit into from
May 20, 2016
Merged

replace bytestring with String #16453

merged 1 commit into from
May 20, 2016

Conversation

StefanKarpinski
Copy link
Member

@StefanKarpinski StefanKarpinski commented May 19, 2016 •
edited
Loading

Part of #16107.

nalimilan
nalimilan reviewed May 19, 2016
View reviewed changes
base/REPL.jl
@@ -271,25 +271,25 @@ end

immutable LatexCompletions <: CompletionProvider; end

bytestring_beforecursor(buf::IOBuffer) = bytestring(buf.data[1:buf.ptr-1])
String_beforecursor(buf::IOBuffer) = String(buf.data[1:buf.ptr-1])
Copy link
Member

Choose a reason for hiding this comment

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

Looks like automated replacement could use some human thinking here. :-)

@nalimilan
Copy link
Member

Shouldn't we keep a deprecated version of bytestring? Or am I missing something?

@StefanKarpinski
Copy link
Member Author

Or am I missing something?

That this is a WIP PR?

@StefanKarpinski StefanKarpinski mentioned this pull request May 19, 2016
Closed
32 tasks
tkelman
tkelman reviewed May 19, 2016
View reviewed changes
base/strings/basic.jl

function bytestring(p::Union{Ptr{UInt8},Ptr{Int8}},len::Integer)
Create a string from the address of a C (0-terminated) string encoded as UTF-8.
A copy is made so the ptr can be safely freed. If `length` is specified, the string
Copy link
Contributor

Choose a reason for hiding this comment

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

spell out pointer?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is cut-and-pasted from the original docs. Rewriting them is another issue.

Copy link
Contributor

@tkelman tkelman May 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Ah. Did not notice that because deleting the old copy isn't in the diff yet. (maybe it was and I just didn't search for what I thought I did)

@tkelman
Copy link
Contributor

tkelman commented May 19, 2016

It's on the list at #16107, but these should probably have NEWS entries as you go.

Some remaining occurrences outside of base:

contrib/BBEditTextWrangler-julia.plist:241:            <string>bytestring</string>
contrib/julia-config.jl:13:    bytestring(opts.image_file);
doc/manual/calling-c-and-fortran-code.rst:88:    julia> bytestring(path)
doc/manual/calling-c-and-fortran-code.rst:120:      bytestring(val)
doc/manual/calling-c-and-fortran-code.rst:144:      return bytestring(pointer(hostname))
doc/manual/calling-c-and-fortran-code.rst:888::func:`unsafe_load` and :func:`bytestring` make copies of data instead of taking ownership
doc/stdlib/strings.rst:53:.. function:: bytestring(::Ptr{UInt8}, [length])
doc/stdlib/strings.rst:59:.. function:: bytestring(s)
examples/clustermanager/0mq/ZMQCM.jl:137:        zid = parse(Int,bytestring(ZMQ.recv(manager.sub)))
examples/clustermanager/0mq/ZMQCM.jl:140:        from_zid = parse(Int,bytestring(ZMQ.recv(manager.sub)))
examples/clustermanager/0mq/ZMQCM.jl:141:        mtype = bytestring(ZMQ.recv(manager.sub))
examples/clustermanager/0mq/ZMQCM.jl:147:            cmsg = bytestring(data)
src/common_symbols2.inc:215:jl_symbol("bytestring"),
src/jlapi.c:110:JL_DLLEXPORT const char *jl_bytestring_ptr(jl_value_t *s)
src/julia.h:1172:JL_DLLEXPORT const char *jl_bytestring_ptr(jl_value_t *s);

@StefanKarpinski
Copy link
Member Author

Thanks, @tkelman. I'm going to write NEWS when the changes are done.

tkelman
tkelman reviewed May 19, 2016
View reviewed changes
contrib/BBEditTextWrangler-julia.plist
@@ -238,7 +238,7 @@
<string>broadcast_setindex!</string>
<string>bswap</string>
<string>bytes2hex</string>
<string>bytestring</string>
<string>String</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

already present in a separate list of types later in the file. if it weren't, would be nice to keep this file sorted

@StefanKarpinski StefanKarpinski changed the title WIP: replace bytestring with String replace bytestring with String May 20, 2016
@StefanKarpinski StefanKarpinski merged commit 5956deb into master May 20, 2016
@StefanKarpinski StefanKarpinski deleted the sk/highlander3 branch May 20, 2016 02:34
@vtjnash
Copy link
Member

vtjnash commented May 20, 2016

Is this the cause of the new failure in genstdlib? It looks like there's also a pending update for one of the definitions in stdlib/string.rst

WARNING: missing docs for signature:

    String(::Ptr{UInt8}, [length])

@tkelman
Copy link
Contributor

tkelman commented May 20, 2016

067ff81 - though I'm going to take a look at some of the things sphinx is complaining about and make more of a branch out of it

tkelman
tkelman reviewed May 21, 2016
View reviewed changes
base/strings/basic.jl
Wrap a vector of bytes encoding string data as UTF-8 in a `String` object.
The resulting `String` object takes ownership of the array.
"""
String(s::Vector{UInt8}) =
Copy link
Contributor

Choose a reason for hiding this comment

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

This gives a method overwrite warning with the inner constructor from base/boot.jl, but removing this method seems to make the "read" test segfault.

Copy link
Member

Choose a reason for hiding this comment

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

That's because one of the calls to String(bytes) in datafmt.jl should have been String(copy(bytes)) ... I have a patch forthcoming.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in #16731.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, @stevengj.

@quinnj
Copy link
Member

quinnj commented May 21, 2016

Does this need something else in Compat.jl? Just doing using Compat doesn't seem to allow 0.4/0.5 compat.

@quinnj
Copy link
Member

quinnj commented May 21, 2016

Yeah, looks like we need a definition like

String(ptr::Ptr, len) = bytestring(ptr, len)

since we didn't have ASCIIString(ptr, len) definitions in 0.4.

@quinnj quinnj mentioned this pull request May 21, 2016
Closed
@stevengj stevengj mentioned this pull request Jun 3, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@StefanKarpinski @nalimilan @tkelman @vtjnash @quinnj @stevengj