Copilot commented on code in PR #3702:
URL: https://github.com/apache/thrift/pull/3702#discussion_r3720308137
##########
lib/rb/lib/thrift/transport/memory_buffer_transport.rb:
##########
@@ -23,13 +23,8 @@ module Thrift
class MemoryBufferTransport < BaseTransport
GARBAGE_BUFFER_SIZE = 4*(2**10) # 4kB
- # If you pass a string to this, you should #dup that string
- # unless you want it to be modified by #read and #write
- #--
- # this behavior is no longer required. If you wish to change it
- # go ahead, just make sure the specs pass
def initialize(buffer = nil)
- @buf = buffer ? Bytes.force_binary_encoding(buffer) :
Bytes.empty_byte_buffer
+ @buf = buffer ? Bytes.force_binary_encoding(buffer.dup) :
Bytes.empty_byte_buffer
Review Comment:
`buffer.dup` can raise `TypeError` for non-duplicable inputs (e.g., if a
caller passes an object that quacks like a buffer but doesn't support `dup`).
Previously, `Bytes.force_binary_encoding(buffer)` would likely raise a
different/clearer error (or possibly coerce). Consider restricting/validating
the accepted input type (e.g., require `String`/`String`-like explicitly) and
raising an `ArgumentError` with a clear message, or coercing via
`String(buffer)`/`to_str` before duping.
##########
lib/rb/lib/thrift/transport/memory_buffer_transport.rb:
##########
@@ -23,13 +23,8 @@ module Thrift
class MemoryBufferTransport < BaseTransport
GARBAGE_BUFFER_SIZE = 4*(2**10) # 4kB
Review Comment:
The constructor behavior changed from shared-buffer to private ownership,
but the prior inline comment was removed entirely. Please add a short doc
comment explaining the current contract (input is duplicated and forced to
binary; subsequent transport reads/writes won't mutate the caller's string;
callers wanting a live view must read from the transport). This prevents
regressions and clarifies the breaking behavioral change for users reading this
class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
