Repository navigation
Rename StaticArray#to_slice to #to_unsafe_slice - #14613
straight-shoota wants to merge 5 commits into
Conversation
| (0...5).each { |i| array[i] = 1_u8 } | ||
| assert_prints base64_encode(array), "AQEBAQE=\n" | ||
| assert_prints base64_strict_encode(array), "AQEBAQE=" | ||
| it "encodes calling `#to_slice`" do |
There was a problem hiding this comment.
I think this test was check that base64 can be called with StaticArray, why remove?
There was a problem hiding this comment.
I interpret the intention of this spec to test that it works with a value that responds to #to_slice. StaticArray was just chosen as an example with a useful #to_slice method. This change respect that intention by replacing StaticArray with String.
I suppose we could add another spec to ensure StaticArray specifically keeps working as well. However this is only going to work as long as deprecated StaticArray#to_slice stays available. I don't think it makes sense to add an explicit overload StaticArray because passing the entire instance as argument is quite heavy and it would be easier to pass a reference, i.e. call to_unsafe_slice at the call site.
ysbaddaden
left a comment
ysbaddaden
left a comment
There was a problem hiding this comment.
I'm gonna have to patch a good number of shards, but 👍
|
@ysbaddaden Yeah it would be super useful to a semantically aware tool to automatically rewrite calls to deprecated methods 🤔 |
|
Interpreter specs are broken due to #14615 |
StaticArray#to_sliceis unsafe because it returns an unprotected pointer to data on the stack. This pointer can easily be overriden which invalidates the slice (see #14610).Renaming to
#to_unsafe_slicehighlights the unsafe nature of using the method, and distinguishes it from other#to_sliceimplementations that are inherently more safe because they return heap pointers.StaticArray#to_slicebecomes deprecated.The first commit replaces
StaticArraywhere it is used in the spec suite as an example for the implementation of#to_slice. Using aStringliteral with the same payload seems like a good idea. It even simplifies those specs.There are quite a lot of calls to
StaticArray#to_slicein the standard library and spec suite. All of them get replaced. It's interesting to note that they're all explicit calls toStaticArray#to_sliceand do not involve dynamic dispatch to some other#to_slicemethod. This makes rewriting easy and confirms that havingStaticArraybreak from the unified#to_sliceinterface does not cause any practical problems. Calls toStaticArray#to_sliceare typically unrelated to calls to other#to_slicemethods.Replaces part of #11031 and resolves part of #9606
Depends on #14615