jorgecarleitao commented on a change in pull request #9044:
URL: https://github.com/apache/arrow/pull/9044#discussion_r554457330
##########
File path: rust/arrow/src/buffer.rs
##########
@@ -756,15 +738,15 @@ impl MutableBuffer {
/// also ensure the new capacity will be a multiple of 64 bytes.
///
/// Returns the new capacity for this buffer.
- pub fn reserve(&mut self, capacity: usize) -> usize {
- if capacity > self.capacity {
- let new_capacity = bit_util::round_upto_multiple_of_64(capacity);
- let new_capacity = cmp::max(new_capacity, self.capacity * 2);
+ pub fn reserve(&mut self, additional: usize) {
Review comment:
Yap, and I think it was a good decision back then, when the goal was to
port C++ code to Rust. I understand your concern.
However, IMO we should not keep using C++-motivated APIs because it was
historically like that. `additional` is the standard in virtually every
container in Rust (`HashMap`, `Vec`, `tokio::bytes::Bytes`). IMO this change
will need to be made at some point, and delaying it will only cause more pain
in the future.
In this particular instance, it was motivated by an already existing
confusion on our own code base, which IMO evidences that rust developers expect
`additional`. Most people that use this API are probably passing `additional`
to it already.
Regardless, we are bumping the major version, so there is already no
expectation of backward compatibility as far as cargo and semver is concerned.
My goal here is to make `MutableBuffer` as close as possible to `Vec` so that
people can use them without even thinking that they are using a special
allocation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
