Jefffrey commented on code in PR #10317:
URL: https://github.com/apache/arrow-rs/pull/10317#discussion_r3589455945
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -121,6 +146,12 @@ impl MutableBuffer {
Self::with_capacity(capacity)
}
+ /// Fallible version of [`MutableBuffer::new`].
+ #[inline]
+ pub fn try_new(capacity: usize) -> Result<Self, MutableBufferError> {
Review Comment:
i wonder if we should bother with having a `try_new` if it just delegates to
`try_with_capacity` 🤔
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -140,13 +174,13 @@ impl MutableBuffer {
NonNull::new(raw_ptr).unwrap_or_else(||
handle_alloc_error(layout))
Review Comment:
are there any downstream concerns with replacing what we previously did
(calling `handle_alloc_error()`) with just an error and relying on downstream
to properly handle (in this case theyd just do a regular panic now)
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -313,74 +406,22 @@ impl MutableBuffer {
slice_to_repeat: &[T],
repeat_count: usize,
) {
- if repeat_count == 0 || slice_to_repeat.is_empty() {
- return;
- }
-
- let bytes_to_repeat = size_of_val(slice_to_repeat);
- let repeated_bytes = repeat_count
- .checked_mul(bytes_to_repeat)
- .expect("repeated slice byte length overflow");
- self.len
- .checked_add(repeated_bytes)
- .expect("mutable buffer length overflow");
-
- // Ensure capacity
- self.reserve(repeated_bytes);
-
- // Save the length before we do all the copies to know where to start
from
- let length_before = self.len;
-
- // Copy the initial slice once so we can use doubling strategy on it
- self.extend_from_slice(slice_to_repeat);
-
- // This tracks how much bytes we have added by repeating so far
- let added_repeats_length = bytes_to_repeat;
- assert_eq!(
- self.len - length_before,
- added_repeats_length,
- "should copy exactly the same number of bytes"
- );
-
- // Number of times the slice was repeated
- let mut already_repeated_times = 1;
-
- // We will use doubling strategy to fill the buffer in
log(repeat_count) steps
- while already_repeated_times < repeat_count {
- // How many slices can we copy in this iteration
- // (either double what we have, or just the remaining ones)
- let number_of_slices_to_copy =
- already_repeated_times.min(repeat_count -
already_repeated_times);
- let number_of_bytes_to_copy = number_of_slices_to_copy *
bytes_to_repeat;
-
- unsafe {
- // Get to the start of the data before we started copying
anything
- let src = self.data.as_ptr().add(length_before) as *const u8;
-
- // Go to the current location to copy to (end of current data)
- let dst = self.data.as_ptr().add(self.len);
-
- // SAFETY: the pointers are not overlapping as there is
`number_of_bytes_to_copy` or less between them
Review Comment:
we probably should preserve some of these comments; especially the safety
one at the very least
##########
arrow-buffer/src/buffer/mutable.rs:
##########
@@ -129,24 +160,35 @@ impl MutableBuffer {
/// then `isize::MAX`, then this function will panic.
#[inline]
pub fn with_capacity(capacity: usize) -> Self {
- let capacity = bit_util::round_upto_multiple_of_64(capacity);
+ Self::try_with_capacity(capacity).unwrap_or_else(|e| panic!("{e}"))
+ }
+
+ /// Fallible version of [`MutableBuffer::with_capacity`].
+ #[inline]
+ pub fn try_with_capacity(capacity: usize) -> Result<Self,
MutableBufferError> {
+ let capacity = capacity
+ .checked_next_multiple_of(64)
Review Comment:
`checked_next_multiple_of` is quite nice; i wonder if that means we can
deprecate `round_upto_multiple_of_64` 🤔 (in a followup)
--
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]
