Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
String: remove to_unsafe_slice and keep to_slice, but make it ret…
…urn a read-only Slice.

Making `String#to_slice` return a copy of the bytes is not good because it wastes memory. It's better to return the original bytes, as before. However, these bytes shouldn't be mutated. To solve this we could introduce `const` to the language, but that's a huge change and it's not clear how to do it. Instead, we let slices be optionally read-only and raise when trying to modify them. This solves the problem because there's no longer unsafe code by invoking `String#to_slice` and it's as efficient as before. We also don't need `Slice.unsafe_readonly` anymore.
  • Loading branch information
asterite committed Feb 20, 2017
1 parent d94bb7e commit bdc81db
Show file tree
Hide file tree
Showing 19 changed files with 97 additions and 115 deletions.
12 changes: 12 additions & 0 deletions spec/std/slice_spec.cr
Expand Up @@ -366,6 +366,18 @@ describe "Slice" do
slice = Slice(Int32).empty
slice.empty?.should be_true
end

it "creates read-only slice" do
slice = Slice.new(3, 0, read_only: true)
expect_raises { slice[0] = 1 }
expect_raises { slice.copy_from(slice) }

subslice = slice[0, 1]
expect_raises { subslice[0] = 1 }

slice = Bytes[1, 2, 3, read_only: true]
expect_raises { slice[0] = 0_u8 }
end
end

private def itself(*args)
Expand Down
2 changes: 1 addition & 1 deletion src/adler32/adler32.cr
Expand Up @@ -10,7 +10,7 @@ module Adler32
end

def self.update(data, adler32 : UInt32) : UInt32
slice = Slice.unsafe_readonly(data)
slice = data.to_slice
LibZ.adler32(adler32, slice, slice.size).to_u32
end

Expand Down
18 changes: 9 additions & 9 deletions src/base64.cr
Expand Up @@ -42,7 +42,7 @@ module Base64
# Q3J5c3RhbA==
# ```
def encode(data) : String
slice = Slice.unsafe_readonly(data)
slice = data.to_slice
String.new(encode_size(slice.size, new_lines: true)) do |buf|
appender = buf.appender
encode_with_new_lines(slice) { |byte| appender << byte }
Expand All @@ -60,7 +60,7 @@ module Base64
# ```
def encode(data, io : IO)
count = 0
encode_with_new_lines(Slice.unsafe_readonly(data)) do |byte|
encode_with_new_lines(data.to_slice) do |byte|
io.write_byte byte
count += 1
end
Expand All @@ -70,7 +70,7 @@ module Base64

private def encode_with_new_lines(data)
inc = 0
to_base64(Slice.unsafe_readonly(data), CHARS_STD, pad: true) do |byte|
to_base64(data.to_slice, CHARS_STD, pad: true) do |byte|
yield byte
inc += 1
if inc >= LINE_SIZE
Expand Down Expand Up @@ -100,7 +100,7 @@ module Base64
end

private def strict_encode(data, alphabet, pad = false)
slice = Slice.unsafe_readonly(data)
slice = data.to_slice
String.new(encode_size(slice.size)) do |buf|
appender = buf.appender
to_base64(slice, alphabet, pad: pad) { |byte| appender << byte }
Expand All @@ -121,7 +121,7 @@ module Base64

private def strict_encode_to_io_internal(data, io, alphabet, pad)
count = 0
to_base64(Slice.unsafe_readonly(data), alphabet, pad: pad) do |byte|
to_base64(data.to_slice, alphabet, pad: pad) do |byte|
count += 1
io.write_byte byte
end
Expand All @@ -138,7 +138,7 @@ module Base64
# The *padding* parameter defaults to `true`. When `false`, enough `=` characters
# are not added to make the output divisible by 4.
def urlsafe_encode(data, padding = true) : String
slice = Slice.unsafe_readonly(data)
slice = data.to_slice
String.new(encode_size(slice.size)) do |buf|
appender = buf.appender
to_base64(slice, CHARS_SAFE, pad: padding) { |byte| appender << byte }
Expand All @@ -159,7 +159,7 @@ module Base64
# Returns the base64-decoded version of *data* as a `Bytes`.
# This will decode either the normal or urlsafe alphabets.
def decode(data) : Bytes
slice = Slice.unsafe_readonly(data)
slice = data.to_slice
buf = Pointer(UInt8).malloc(decode_size(slice.size))
appender = buf.appender
from_base64(slice) { |byte| appender << byte }
Expand All @@ -170,7 +170,7 @@ module Base64
# This will decode either the normal or urlsafe alphabets.
def decode(data, io : IO)
count = 0
from_base64(Slice.unsafe_readonly(data)) do |byte|
from_base64(data.to_slice) do |byte|
io.write_byte byte
count += 1
end
Expand All @@ -181,7 +181,7 @@ module Base64
# Returns the base64-decoded version of *data* as a string.
# This will decode either the normal or urlsafe alphabets.
def decode_string(data) : String
slice = Slice.unsafe_readonly(data)
slice = data.to_slice
String.new(decode_size(slice.size)) do |buf|
appender = buf.appender
from_base64(slice) { |byte| appender << byte }
Expand Down
2 changes: 1 addition & 1 deletion src/crc32/crc32.cr
Expand Up @@ -10,7 +10,7 @@ module CRC32
end

def self.update(data, crc32 : UInt32) : UInt32
slice = Slice.unsafe_readonly(data)
slice = data.to_slice
LibZ.crc32(crc32, slice, slice.size).to_u32
end

Expand Down
4 changes: 2 additions & 2 deletions src/crypto/subtle.cr
Expand Up @@ -10,8 +10,8 @@ module Crypto::Subtle
#
# NOTE: *x* and *y* must be able to respond to `to_slice`.
def self.constant_time_compare(x, y) : Bool
x = Slice.unsafe_readonly(x)
y = Slice.unsafe_readonly(y)
x = x.to_slice
y = y.to_slice
return false if x.size != y.size

v = 0_u8
Expand Down
4 changes: 2 additions & 2 deletions src/digest/base.cr
Expand Up @@ -4,7 +4,7 @@ abstract class Digest::Base
# Returns the hash of *data*. *data* must respond to `#to_slice`.
def self.digest(data)
digest do |ctx|
ctx.update(Slice.unsafe_readonly(data))
ctx.update(data.to_slice)
end
end

Expand Down Expand Up @@ -50,7 +50,7 @@ abstract class Digest::Base
yield ctx
end

Slice.unsafe_readonly(hashsum).hexstring
hashsum.to_slice.hexstring
end

# Returns the base64-encoded hash of *data*.
Expand Down
2 changes: 1 addition & 1 deletion src/digest/md5.cr
Expand Up @@ -14,7 +14,7 @@ class Digest::MD5 < Digest::Base
end

def update(data)
slice = Slice.unsafe_readonly(data)
slice = data.to_slice
update(slice.to_unsafe, slice.bytesize.to_u32)
end

Expand Down
2 changes: 1 addition & 1 deletion src/digest/sha1.cr
Expand Up @@ -20,7 +20,7 @@ class Digest::SHA1 < Digest::Base
end

def update(data)
message_array = Slice.unsafe_readonly(data)
message_array = data.to_slice
message_array.each do |byte|
@message_block[@message_block_index] = byte & 0xFF_u8
@message_block_index += 1
Expand Down
2 changes: 1 addition & 1 deletion src/http/common.cr
Expand Up @@ -237,7 +237,7 @@ module HTTP
# HTTP.dequote_string(quoted) # => %q("foo\bar")
# ```
def self.dequote_string(str)
data = Slice.unsafe_readonly(str)
data = str.to_slice
quoted_pair_index = data.index('\\'.ord)
return str unless quoted_pair_index

Expand Down
8 changes: 4 additions & 4 deletions src/http/web_socket/protocol.cr
Expand Up @@ -84,7 +84,7 @@ class HTTP::WebSocket::Protocol
end

def send(data : String)
send(data.to_unsafe_slice, Opcode::TEXT)
send(data.to_slice, Opcode::TEXT)
end

def send(data : Bytes)
Expand Down Expand Up @@ -215,23 +215,23 @@ class HTTP::WebSocket::Protocol

def ping(message = nil)
if message
send(Slice.unsafe_readonly(message), Opcode::PING)
send(message.to_slice, Opcode::PING)
else
send(Bytes.empty, Opcode::PING)
end
end

def pong(message = nil)
if message
send(Slice.unsafe_readonly(message), Opcode::PONG)
send(message.to_slice, Opcode::PONG)
else
send(Bytes.empty, Opcode::PONG)
end
end

def close(message = nil)
if message
send(Slice.unsafe_readonly(message), Opcode::CLOSE)
send(message.to_slice, Opcode::CLOSE)
else
send(Bytes.empty, Opcode::CLOSE)
end
Expand Down
24 changes: 5 additions & 19 deletions src/io/memory.cr
Expand Up @@ -68,7 +68,7 @@ class IO::Memory
# io.print "hi" # raises IO::Error
# ```
def self.new(string : String)
new string.to_unsafe_slice, writeable: false
new string.to_slice, writeable: false
end

# See `IO#read(slice)`.
Expand Down Expand Up @@ -361,7 +361,7 @@ class IO::Memory

old_writeable = @writeable
old_resizeable = @resizeable
io = IO::Memory.new(to_unsafe_slice[offset, bytesize], writeable: false)
io = IO::Memory.new(to_slice[offset, bytesize], writeable: false)
begin
@writeable = false
@resizeable = false
Expand Down Expand Up @@ -407,21 +407,7 @@ class IO::Memory
String.new @buffer, @bytesize
end

# Returns a `Slice` over the internal buffer. Modifying the slice
# modifies the internal buffer.
#
# This method is **unsafe** because the internal buffer might
# point to bytes that came from a string literal, which are
# placed in the ROM (read-only memory) section data of the program,
# and attempting to mutate the returned Bytes might result
# in segmentation fault. You should use `to_slice` instead unless
# you know the returned bytes won't be mutated, to save an
# extra memory allocation.
def to_unsafe_slice : Bytes
Slice.new(@buffer, @bytesize)
end

# Returns a copy of the underlying bytes.
# Returns the underlying bytes.
#
# ```
# io = IO::Memory.new
Expand All @@ -430,12 +416,12 @@ class IO::Memory
# io.to_slice # => Bytes[104, 101, 108, 108, 111]
# ```
def to_slice : Bytes
to_unsafe_slice.clone
Slice.new(@buffer, @bytesize, read_only: !@writeable)
end

# Appends this internal buffer to the given `IO`.
def to_s(io)
io.write(to_unsafe_slice)
io.write(to_slice)
end

private def check_writeable
Expand Down
2 changes: 1 addition & 1 deletion src/json/lexer/string_based.cr
Expand Up @@ -55,7 +55,7 @@ class JSON::Lexer::StringBased < JSON::Lexer
end

def slice_range(start_pos, end_pos)
@reader.string.to_unsafe_slice[start_pos, end_pos - start_pos]
@reader.string.to_slice[start_pos, end_pos - start_pos]
end

private def next_char_no_column_increment
Expand Down
4 changes: 2 additions & 2 deletions src/number.cr
Expand Up @@ -44,8 +44,8 @@ struct Number
# ints = Int64.slice(1, 2, 3)
# ints.class # => Slice(Int64)
# ```
macro slice(*nums)
%slice = Slice({{@type}}).new({{nums.size}})
macro slice(*nums, read_only = false)
%slice = Slice({{@type}}).new({{nums.size}}, read_only: {{read_only}})
{% for num, i in nums %}
%slice.to_unsafe[{{i}}] = {{@type}}.new({{num}})
{% end %}
Expand Down
2 changes: 1 addition & 1 deletion src/openssl/cipher.cr
Expand Up @@ -51,7 +51,7 @@ class OpenSSL::Cipher
end

def update(data)
slice = Slice.unsafe_readonly(data)
slice = data.to_slice
buffer_length = slice.size + block_size
buffer = Bytes.new(buffer_length)
if LibCrypto.evp_cipherupdate(@ctx, buffer, pointerof(buffer_length), slice, slice.size) != 1
Expand Down
4 changes: 2 additions & 2 deletions src/openssl/hmac.cr
Expand Up @@ -15,8 +15,8 @@ class OpenSSL::HMAC
when :sha512 then LibCrypto.evp_sha512
else raise "Unsupported digest algorithm: #{algorithm}"
end
key_slice = Slice.unsafe_readonly(key)
data_slice = Slice.unsafe_readonly(data)
key_slice = key.to_slice
data_slice = data.to_slice
buffer = Bytes.new(128)
LibCrypto.hmac(evp, key_slice, key_slice.size, data_slice, data_slice.size, buffer, out buffer_len)
buffer[0, buffer_len.to_i]
Expand Down
2 changes: 1 addition & 1 deletion src/openssl/ssl/context.cr
Expand Up @@ -295,7 +295,7 @@ abstract class OpenSSL::SSL::Context
def alpn_protocol=(protocol : String)
proto = Bytes.new(protocol.bytesize + 1)
proto[0] = protocol.bytesize.to_u8
protocol.to_unsafe_slice.copy_to(proto.to_unsafe + 1, protocol.bytesize)
protocol.to_slice.copy_to(proto.to_unsafe + 1, protocol.bytesize)
self.alpn_protocol = proto
end

Expand Down

1 comment on commit bdc81db

@Sija
Copy link
Contributor

@Sija Sija commented on bdc81db Feb 23, 2017

Choose a reason for hiding this comment

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

The other day I was thinking about similar tweak to get rid of newly introduced call to Slice.unsafe_readonly and now you did it, hooray! :)

Please sign in to comment.