Protect internal String methods #5503
Conversation
src/file.cr
Outdated
| @@ -469,7 +469,8 @@ class File < IO::FileDescriptor | |||
| byte_count -= 1 | |||
| end | |||
|
|
|||
| str.write part.unsafe_byte_slice(byte_start, byte_count) | |||
| pointer = part.to_unsafe + byte_start | |||
| str.write Slice.new(pointer, byte_count, read_only: true) | |||
There was a problem hiding this comment.
str.write part.to_slice[pointer, byte_count] should work...
There was a problem hiding this comment.
Ah, thanks, I’m still learning all of this. 🙂
Unfortunately:
in src/file.cr:472: instantiating 'Slice(UInt8)#[](Pointer(UInt8), Int32)'
str.write part.to_slice[part.to_unsafe + byte_start, byte_count]
^
in src/slice.cr:195: no overload matches 'Int32#<=' with type Pointer(UInt8)
Overloads are:
- Int32#<=(other : Int8)
- Int32#<=(other : Int16)
- Int32#<=(other : Int32)
- Int32#<=(other : Int64)
- Int32#<=(other : Int128)
- Int32#<=(other : UInt8)
- Int32#<=(other : UInt16)
- Int32#<=(other : UInt32)
- Int32#<=(other : UInt64)
- Int32#<=(other : UInt128)
- Int32#<=(other : Float32)
- Int32#<=(other : Float64)
- Comparable(T)#<=(other : T)
unless 0 <= start <= @size
^
There was a problem hiding this comment.
Actually, should this be just the below?
str.write part.to_slice[byte_start, byte_count]
src/string.cr
Outdated
| @@ -900,7 +900,7 @@ class String | |||
| end | |||
| end | |||
|
|
|||
| def unsafe_byte_at(index) | |||
| protected def unsafe_byte_at(index) | |||
There was a problem hiding this comment.
This should be removed and replaced with just to_unsafe[index] where used.
src/string.cr
Outdated
| @@ -900,7 +900,7 @@ class String | |||
| end | |||
| end | |||
|
|
|||
| def unsafe_byte_at(index) | |||
| protected def unsafe_byte_at(index) | |||
There was a problem hiding this comment.
When all calls to this method get replaced by to_unsafe[index] it could as well be removed entirely.
There was a problem hiding this comment.
I think we should really remove this method.
string.to_unsafe[index]
string.unsafe_byte_at_index(index)the second is even longer!
Also, I'd like to come back with the idea of having general benchmarks of the whole standard library. Then we could see which places don't need to be "unsafe" and work just fine with safe code.
chastell
force-pushed
the
protect_internal_String_methods
branch
from
edc0013
to
a7680bf
Compare
chastell
force-pushed
the
protect_internal_String_methods
branch
from
a7680bf
to
7141fa9
Compare
|
Addressed the comments, just need a double-check on whether str.write part.unsafe_byte_slice(byte_start, byte_count)and str.write part.to_slice[byte_start, byte_count]are equivalent (they do look so to me and the tests pass). |
|
The latter is a bit slower because it has to do index bounds check. I don't know how it affects performance, though. That's why we need #5508 to know if this kind of refactors are OK to merge. |
Well I did initially go with pointer = part.to_unsafe + byte_start
str.write Slice.new(pointer, byte_count, read_only: true)but received a change request for |
|
@chastell This needs a rebase. |
|
@chastell Do you still plan working on that PR? |
|
Closing stale PR.
|
This makes
String#unsafe_byte_atandString#unsafe_byte_sliceprotected, as per this comment.