Rename `set_len` to `inc_len` and simplify its safety contract.
---
 rust/kernel/alloc/kvec.rs | 19 +++++++++----------
 rust/kernel/str.rs        |  2 +-
 rust/kernel/uaccess.rs    |  2 +-
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
index ae9d072741ce..d43a1d609434 100644
--- a/rust/kernel/alloc/kvec.rs
+++ b/rust/kernel/alloc/kvec.rs
@@ -183,17 +183,16 @@ pub fn len(&self) -> usize {
         self.len
     }
 
-    /// Forcefully sets `self.len` to `new_len`.
+    /// Increments `self.len` by `additional`.
     ///
     /// # Safety
     ///
-    /// - `new_len` must be less than or equal to [`Self::capacity`].
-    /// - If `new_len` is greater than `self.len`, all elements within the interval
-    ///   [`self.len`,`new_len`) must be initialized.
+    /// - `self.len + additional` must be less than or equal to [`Self::capacity`].
+    /// - All elements within the interval [`self.len`,`self.len + additional`) must be initialized.
     #[inline]
-    pub unsafe fn set_len(&mut self, new_len: usize) {
-        debug_assert!(new_len <= self.capacity());
-        self.len = new_len;
+    pub unsafe fn inc_len(&mut self, additional: usize) {
+        debug_assert!(self.len() + additional <= self.capacity());
+        self.len += additional;
     }
 
     /// Returns a slice of the entire vector.
@@ -298,7 +297,7 @@ pub fn push(&mut self, v: T, flags: Flags) -> Result<(), AllocError> {
         // SAFETY: We just initialised the first spare entry, so it is safe to increase the length
         // by 1. We also know that the new length is <= capacity because of the previous call to
         // `reserve` above.
-        unsafe { self.set_len(self.len() + 1) };
+        unsafe { self.inc_len(1) };
         Ok(())
     }
 
@@ -475,7 +474,7 @@ pub fn extend_with(&mut self, n: usize, value: T, flags: Flags) -> Result<(), Al
         // SAFETY:
         // - `self.len() + n < self.capacity()` due to the call to reserve above,
         // - the loop and the line above initialized the next `n` elements.
-        unsafe { self.set_len(self.len() + n) };
+        unsafe { self.inc_len(n) };
 
         Ok(())
     }
@@ -506,7 +505,7 @@ pub fn extend_from_slice(&mut self, other: &[T], flags: Flags) -> Result<(), All
         //   the length by the same number.
         // - `self.len() + other.len() <= self.capacity()` is guaranteed by the preceding `reserve`
         //   call.
-        unsafe { self.set_len(self.len() + other.len()) };
+        unsafe { self.inc_len(other.len()) };
         Ok(())
     }
 
diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
index 28e2201604d6..005713839e9e 100644
--- a/rust/kernel/str.rs
+++ b/rust/kernel/str.rs
@@ -840,7 +840,7 @@ pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> {
 
         // SAFETY: The number of bytes that can be written to `f` is bounded by `size`, which is
         // `buf`'s capacity. The contents of the buffer have been initialised by writes to `f`.
-        unsafe { buf.set_len(f.bytes_written()) };
+        unsafe { buf.inc_len(f.bytes_written()) };
 
         // Check that there are no `NUL` bytes before the end.
         // SAFETY: The buffer is valid for read because `f.bytes_written()` is bounded by `size`
diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
index 719b0a48ff55..0aa5455a18be 100644
--- a/rust/kernel/uaccess.rs
+++ b/rust/kernel/uaccess.rs
@@ -291,7 +291,7 @@ pub fn read_all<A: Allocator>(mut self, buf: &mut Vec<u8, A>, flags: Flags) -> R
 
         // SAFETY: Since the call to `read_raw` was successful, so the next `len` bytes of the
         // vector have been initialized.
-        unsafe { buf.set_len(buf.len() + len) };
+        unsafe { buf.inc_len(len) };
         Ok(())
     }
 }

-- 
2.48.1

On Sun, Mar 16, 2025 at 06:32:00PM -0400, Tamir Duberstein wrote:
> Rename `set_len` to `inc_len` and simplify its safety contract.

You're missing a Signed-off-by tag.

>  rust/kernel/alloc/kvec.rs | 19 +++++++++----------
>  rust/kernel/str.rs        |  2 +-
>  rust/kernel/uaccess.rs    |  2 +-
>  3 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index ae9d072741ce..d43a1d609434 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -183,17 +183,16 @@ pub fn len(&self) -> usize {
>          self.len
>      }
>  
> -    /// Forcefully sets `self.len` to `new_len`.
> +    /// Increments `self.len` by `additional`.
>      ///
>      /// # Safety
>      ///
> -    /// - `new_len` must be less than or equal to [`Self::capacity`].
> -    /// - If `new_len` is greater than `self.len`, all elements within the interval
> -    ///   [`self.len`,`new_len`) must be initialized.
> +    /// - `self.len + additional` must be less than or equal to [`Self::capacity`].
> +    /// - All elements within the interval [`self.len`,`self.len + additional`) must be initialized.
>      #[inline]
> -    pub unsafe fn set_len(&mut self, new_len: usize) {
> -        debug_assert!(new_len <= self.capacity());
> -        self.len = new_len;
> +    pub unsafe fn inc_len(&mut self, additional: usize) {
> +        debug_assert!(self.len() + additional <= self.capacity());
> +        self.len += additional;

I guess we could use an INVARIANT: comment here.

Alice

On Mon, Mar 17, 2025 at 6:50 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Sun, Mar 16, 2025 at 06:32:00PM -0400, Tamir Duberstein wrote:
> > Rename `set_len` to `inc_len` and simplify its safety contract.
>
> You're missing a Signed-off-by tag.

Thanks, added.

On Mon, Mar 17, 2025 at 10:50:15AM +0000, Alice Ryhl wrote:
> On Sun, Mar 16, 2025 at 06:32:00PM -0400, Tamir Duberstein wrote:
> > Rename `set_len` to `inc_len` and simplify its safety contract.
> 
> You're missing a Signed-off-by tag.
> 
> >  rust/kernel/alloc/kvec.rs | 19 +++++++++----------
> >  rust/kernel/str.rs        |  2 +-
> >  rust/kernel/uaccess.rs    |  2 +-
> >  3 files changed, 11 insertions(+), 12 deletions(-)
> > 
> > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > index ae9d072741ce..d43a1d609434 100644
> > --- a/rust/kernel/alloc/kvec.rs
> > +++ b/rust/kernel/alloc/kvec.rs
> > @@ -183,17 +183,16 @@ pub fn len(&self) -> usize {
> >          self.len
> >      }
> >  
> > -    /// Forcefully sets `self.len` to `new_len`.
> > +    /// Increments `self.len` by `additional`.
> >      ///
> >      /// # Safety
> >      ///
> > -    /// - `new_len` must be less than or equal to [`Self::capacity`].
> > -    /// - If `new_len` is greater than `self.len`, all elements within the interval
> > -    ///   [`self.len`,`new_len`) must be initialized.
> > +    /// - `self.len + additional` must be less than or equal to [`Self::capacity`].
> > +    /// - All elements within the interval [`self.len`,`self.len + additional`) must be initialized.
> >      #[inline]
> > -    pub unsafe fn set_len(&mut self, new_len: usize) {
> > -        debug_assert!(new_len <= self.capacity());
> > -        self.len = new_len;
> > +    pub unsafe fn inc_len(&mut self, additional: usize) {
> > +        debug_assert!(self.len() + additional <= self.capacity());
> > +        self.len += additional;
> 
> I guess we could use an INVARIANT: comment here.

Yeah, I fixed that up in a separate patch. I'm fine with Tamir picking it up or
doing it himself in a new one, etc. But I think this patch should only focus on
the rename.

On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> Rename `set_len` to `inc_len` and simplify its safety contract.
> ---
>  rust/kernel/alloc/kvec.rs | 19 +++++++++----------
>  rust/kernel/str.rs        |  2 +-
>  rust/kernel/uaccess.rs    |  2 +-
>  3 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> index ae9d072741ce..d43a1d609434 100644
> --- a/rust/kernel/alloc/kvec.rs
> +++ b/rust/kernel/alloc/kvec.rs
> @@ -183,17 +183,16 @@ pub fn len(&self) -> usize {
>          self.len
>      }
>  
> -    /// Forcefully sets `self.len` to `new_len`.
> +    /// Increments `self.len` by `additional`.

I would keep the "Forcefully".

>      ///
>      /// # Safety
>      ///
> -    /// - `new_len` must be less than or equal to [`Self::capacity`].
> -    /// - If `new_len` is greater than `self.len`, all elements within the interval
> -    ///   [`self.len`,`new_len`) must be initialized.
> +    /// - `self.len + additional` must be less than or equal to [`Self::capacity`].
> +    /// - All elements within the interval [`self.len`,`self.len + additional`) must be initialized.
>      #[inline]
> -    pub unsafe fn set_len(&mut self, new_len: usize) {
> -        debug_assert!(new_len <= self.capacity());
> -        self.len = new_len;
> +    pub unsafe fn inc_len(&mut self, additional: usize) {
> +        debug_assert!(self.len() + additional <= self.capacity());

What if this overflows? Do we always have overflow debugging on when
debug assertions are enabled? If yes, then this is fine.

> +        self.len += additional;
>      }
>  
>      /// Returns a slice of the entire vector.

> diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> index 28e2201604d6..005713839e9e 100644
> --- a/rust/kernel/str.rs
> +++ b/rust/kernel/str.rs
> @@ -840,7 +840,7 @@ pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> {
>  
>          // SAFETY: The number of bytes that can be written to `f` is bounded by `size`, which is
>          // `buf`'s capacity. The contents of the buffer have been initialised by writes to `f`.
> -        unsafe { buf.set_len(f.bytes_written()) };
> +        unsafe { buf.inc_len(f.bytes_written()) };

This change seems wrong unless the code was wrong to begin with.

Otherwise the change looks good.

---
Cheers,
Benno

>  
>          // Check that there are no `NUL` bytes before the end.
>          // SAFETY: The buffer is valid for read because `f.bytes_written()` is bounded by `size`
> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> index 719b0a48ff55..0aa5455a18be 100644
> --- a/rust/kernel/uaccess.rs
> +++ b/rust/kernel/uaccess.rs
> @@ -291,7 +291,7 @@ pub fn read_all<A: Allocator>(mut self, buf: &mut Vec<u8, A>, flags: Flags) -> R
>  
>          // SAFETY: Since the call to `read_raw` was successful, so the next `len` bytes of the
>          // vector have been initialized.
> -        unsafe { buf.set_len(buf.len() + len) };
> +        unsafe { buf.inc_len(len) };
>          Ok(())
>      }
>  }

On Mon, Mar 17, 2025 at 09:58:35AM +0000, Benno Lossin wrote:
> On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> > Rename `set_len` to `inc_len` and simplify its safety contract.
> > ---
> >  rust/kernel/alloc/kvec.rs | 19 +++++++++----------
> >  rust/kernel/str.rs        |  2 +-
> >  rust/kernel/uaccess.rs    |  2 +-
> >  3 files changed, 11 insertions(+), 12 deletions(-)
> >
> > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > index ae9d072741ce..d43a1d609434 100644
> > --- a/rust/kernel/alloc/kvec.rs
> > +++ b/rust/kernel/alloc/kvec.rs
> > @@ -183,17 +183,16 @@ pub fn len(&self) -> usize {
> >          self.len
> >      }
> >  
> > -    /// Forcefully sets `self.len` to `new_len`.
> > +    /// Increments `self.len` by `additional`.
> 
> I would keep the "Forcefully".
> 
> >      ///
> >      /// # Safety
> >      ///
> > -    /// - `new_len` must be less than or equal to [`Self::capacity`].
> > -    /// - If `new_len` is greater than `self.len`, all elements within the interval
> > -    ///   [`self.len`,`new_len`) must be initialized.
> > +    /// - `self.len + additional` must be less than or equal to [`Self::capacity`].
> > +    /// - All elements within the interval [`self.len`,`self.len + additional`) must be initialized.
> >      #[inline]
> > -    pub unsafe fn set_len(&mut self, new_len: usize) {
> > -        debug_assert!(new_len <= self.capacity());
> > -        self.len = new_len;
> > +    pub unsafe fn inc_len(&mut self, additional: usize) {
> > +        debug_assert!(self.len() + additional <= self.capacity());
> 
> What if this overflows? Do we always have overflow debugging on when
> debug assertions are enabled? If yes, then this is fine.

I don't think we do.

> > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> > index 28e2201604d6..005713839e9e 100644
> > --- a/rust/kernel/str.rs
> > +++ b/rust/kernel/str.rs
> > @@ -840,7 +840,7 @@ pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> {
> >  
> >          // SAFETY: The number of bytes that can be written to `f` is bounded by `size`, which is
> >          // `buf`'s capacity. The contents of the buffer have been initialised by writes to `f`.
> > -        unsafe { buf.set_len(f.bytes_written()) };
> > +        unsafe { buf.inc_len(f.bytes_written()) };
> 
> This change seems wrong unless the code was wrong to begin with.
> 
> Otherwise the change looks good.

The buffer has length zero as it was just created with:

let mut buf = KVec::with_capacity(size, GFP_KERNEL)?;

Alice

On Mon, Mar 17, 2025 at 6:48 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Mon, Mar 17, 2025 at 09:58:35AM +0000, Benno Lossin wrote:
> > On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> > > Rename `set_len` to `inc_len` and simplify its safety contract.
> > > ---
> > >  rust/kernel/alloc/kvec.rs | 19 +++++++++----------
> > >  rust/kernel/str.rs        |  2 +-
> > >  rust/kernel/uaccess.rs    |  2 +-
> > >  3 files changed, 11 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> > > index ae9d072741ce..d43a1d609434 100644
> > > --- a/rust/kernel/alloc/kvec.rs
> > > +++ b/rust/kernel/alloc/kvec.rs
> > > @@ -183,17 +183,16 @@ pub fn len(&self) -> usize {
> > >          self.len
> > >      }
> > >
> > > -    /// Forcefully sets `self.len` to `new_len`.
> > > +    /// Increments `self.len` by `additional`.
> >
> > I would keep the "Forcefully".

Why? Is it possible to set it any other way?

> >
> > >      ///
> > >      /// # Safety
> > >      ///
> > > -    /// - `new_len` must be less than or equal to [`Self::capacity`].
> > > -    /// - If `new_len` is greater than `self.len`, all elements within the interval
> > > -    ///   [`self.len`,`new_len`) must be initialized.
> > > +    /// - `self.len + additional` must be less than or equal to [`Self::capacity`].
> > > +    /// - All elements within the interval [`self.len`,`self.len + additional`) must be initialized.
> > >      #[inline]
> > > -    pub unsafe fn set_len(&mut self, new_len: usize) {
> > > -        debug_assert!(new_len <= self.capacity());
> > > -        self.len = new_len;
> > > +    pub unsafe fn inc_len(&mut self, additional: usize) {
> > > +        debug_assert!(self.len() + additional <= self.capacity());
> >
> > What if this overflows? Do we always have overflow debugging on when
> > debug assertions are enabled? If yes, then this is fine.
>
> I don't think we do.

Rearranged as

        debug_assert!(additional <= self.capacity() - self.len());

It should be impossible for this to underflow because capacity must be
>= len. Interestingly this isn't a documented invariant, but it is
relied upon by `spare_capacity_mut`.

> > > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
> > > index 28e2201604d6..005713839e9e 100644
> > > --- a/rust/kernel/str.rs
> > > +++ b/rust/kernel/str.rs
> > > @@ -840,7 +840,7 @@ pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> {
> > >
> > >          // SAFETY: The number of bytes that can be written to `f` is bounded by `size`, which is
> > >          // `buf`'s capacity. The contents of the buffer have been initialised by writes to `f`.
> > > -        unsafe { buf.set_len(f.bytes_written()) };
> > > +        unsafe { buf.inc_len(f.bytes_written()) };
> >
> > This change seems wrong unless the code was wrong to begin with.
> >
> > Otherwise the change looks good.
>
> The buffer has length zero as it was just created with:
>
> let mut buf = KVec::with_capacity(size, GFP_KERNEL)?;

Indeed. Added to the commit message.

On Mon Mar 17, 2025 at 12:25 PM CET, Tamir Duberstein wrote:
> On Mon, Mar 17, 2025 at 6:48 AM Alice Ryhl <aliceryhl@google.com> wrote:
>>
>> On Mon, Mar 17, 2025 at 09:58:35AM +0000, Benno Lossin wrote:
>> > On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
>> > > Rename `set_len` to `inc_len` and simplify its safety contract.
>> > > ---
>> > >  rust/kernel/alloc/kvec.rs | 19 +++++++++----------
>> > >  rust/kernel/str.rs        |  2 +-
>> > >  rust/kernel/uaccess.rs    |  2 +-
>> > >  3 files changed, 11 insertions(+), 12 deletions(-)
>> > >
>> > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
>> > > index ae9d072741ce..d43a1d609434 100644
>> > > --- a/rust/kernel/alloc/kvec.rs
>> > > +++ b/rust/kernel/alloc/kvec.rs
>> > > @@ -183,17 +183,16 @@ pub fn len(&self) -> usize {
>> > >          self.len
>> > >      }
>> > >
>> > > -    /// Forcefully sets `self.len` to `new_len`.
>> > > +    /// Increments `self.len` by `additional`.
>> >
>> > I would keep the "Forcefully".
>
> Why? Is it possible to set it any other way?

Yeah when `truncate` and `resize` land. It conveys that this is a
low-level operation.

>> > >      ///
>> > >      /// # Safety
>> > >      ///
>> > > -    /// - `new_len` must be less than or equal to [`Self::capacity`].
>> > > -    /// - If `new_len` is greater than `self.len`, all elements within the interval
>> > > -    ///   [`self.len`,`new_len`) must be initialized.
>> > > +    /// - `self.len + additional` must be less than or equal to [`Self::capacity`].
>> > > +    /// - All elements within the interval [`self.len`,`self.len + additional`) must be initialized.
>> > >      #[inline]
>> > > -    pub unsafe fn set_len(&mut self, new_len: usize) {
>> > > -        debug_assert!(new_len <= self.capacity());
>> > > -        self.len = new_len;
>> > > +    pub unsafe fn inc_len(&mut self, additional: usize) {
>> > > +        debug_assert!(self.len() + additional <= self.capacity());
>> >
>> > What if this overflows? Do we always have overflow debugging on when
>> > debug assertions are enabled? If yes, then this is fine.
>>
>> I don't think we do.
>
> Rearranged as
>
>         debug_assert!(additional <= self.capacity() - self.len());

LGTM

> It should be impossible for this to underflow because capacity must be
>>= len. Interestingly this isn't a documented invariant, but it is
> relied upon by `spare_capacity_mut`.

Oh yeah that should be an invariant. Feel free to open an issue or send
a patch.

>> > > diff --git a/rust/kernel/str.rs b/rust/kernel/str.rs
>> > > index 28e2201604d6..005713839e9e 100644
>> > > --- a/rust/kernel/str.rs
>> > > +++ b/rust/kernel/str.rs
>> > > @@ -840,7 +840,7 @@ pub fn try_from_fmt(args: fmt::Arguments<'_>) -> Result<Self, Error> {
>> > >
>> > >          // SAFETY: The number of bytes that can be written to `f` is bounded by `size`, which is
>> > >          // `buf`'s capacity. The contents of the buffer have been initialised by writes to `f`.
>> > > -        unsafe { buf.set_len(f.bytes_written()) };
>> > > +        unsafe { buf.inc_len(f.bytes_written()) };
>> >
>> > This change seems wrong unless the code was wrong to begin with.
>> >
>> > Otherwise the change looks good.
>>
>> The buffer has length zero as it was just created with:
>>
>> let mut buf = KVec::with_capacity(size, GFP_KERNEL)?;

Ahh, I didn't look at the context. Makes sense.

> Indeed. Added to the commit message.

Thanks.

---
Cheers,
Benno

On Mon, Mar 17, 2025 at 10:46 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> On Mon Mar 17, 2025 at 12:25 PM CET, Tamir Duberstein wrote:
> > On Mon, Mar 17, 2025 at 6:48 AM Alice Ryhl <aliceryhl@google.com> wrote:
> >>
> >> On Mon, Mar 17, 2025 at 09:58:35AM +0000, Benno Lossin wrote:
> >> > On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> >> > > Rename `set_len` to `inc_len` and simplify its safety contract.
> >> > > ---
> >> > >  rust/kernel/alloc/kvec.rs | 19 +++++++++----------
> >> > >  rust/kernel/str.rs        |  2 +-
> >> > >  rust/kernel/uaccess.rs    |  2 +-
> >> > >  3 files changed, 11 insertions(+), 12 deletions(-)
> >> > >
> >> > > diff --git a/rust/kernel/alloc/kvec.rs b/rust/kernel/alloc/kvec.rs
> >> > > index ae9d072741ce..d43a1d609434 100644
> >> > > --- a/rust/kernel/alloc/kvec.rs
> >> > > +++ b/rust/kernel/alloc/kvec.rs
> >> > > @@ -183,17 +183,16 @@ pub fn len(&self) -> usize {
> >> > >          self.len
> >> > >      }
> >> > >
> >> > > -    /// Forcefully sets `self.len` to `new_len`.
> >> > > +    /// Increments `self.len` by `additional`.
> >> >
> >> > I would keep the "Forcefully".
> >
> > Why? Is it possible to set it any other way?
>
> Yeah when `truncate` and `resize` land. It conveys that this is a
> low-level operation.

It's also an unsafe function whose safety section mentions that the
memory must already be initialized. I don't think this word adds any
value.

>
> >> > >      ///
> >> > >      /// # Safety
> >> > >      ///
> >> > > -    /// - `new_len` must be less than or equal to [`Self::capacity`].
> >> > > -    /// - If `new_len` is greater than `self.len`, all elements within the interval
> >> > > -    ///   [`self.len`,`new_len`) must be initialized.
> >> > > +    /// - `self.len + additional` must be less than or equal to [`Self::capacity`].
> >> > > +    /// - All elements within the interval [`self.len`,`self.len + additional`) must be initialized.
> >> > >      #[inline]
> >> > > -    pub unsafe fn set_len(&mut self, new_len: usize) {
> >> > > -        debug_assert!(new_len <= self.capacity());
> >> > > -        self.len = new_len;
> >> > > +    pub unsafe fn inc_len(&mut self, additional: usize) {
> >> > > +        debug_assert!(self.len() + additional <= self.capacity());
> >> >
> >> > What if this overflows? Do we always have overflow debugging on when
> >> > debug assertions are enabled? If yes, then this is fine.
> >>
> >> I don't think we do.
> >
> > Rearranged as
> >
> >         debug_assert!(additional <= self.capacity() - self.len());
>
> LGTM
>
> > It should be impossible for this to underflow because capacity must be
> >>= len. Interestingly this isn't a documented invariant, but it is
> > relied upon by `spare_capacity_mut`.
>
> Oh yeah that should be an invariant. Feel free to open an issue or send
> a patch.

Will prepend a patch in this series.

On Mon, Mar 17, 2025 at 10:58 AM Benno Lossin <benno.lossin@proton.me> wrote:
>
> What if this overflows? Do we always have overflow debugging on when
> debug assertions are enabled? If yes, then this is fine.

No, they are independent settings.

> This change seems wrong unless the code was wrong to begin with.

Yeah, even if it is correct, it should definitely be described in the
commit log, because it does look like a functional change.

Cheers,
Miguel

On Mon Mar 17, 2025 at 11:23 AM CET, Miguel Ojeda wrote:
> On Mon, Mar 17, 2025 at 10:58 AM Benno Lossin <benno.lossin@proton.me> wrote:
>>
>> What if this overflows? Do we always have overflow debugging on when
>> debug assertions are enabled? If yes, then this is fine.
>
> No, they are independent settings.

I see, then this needs to be `checked_add` or some avoid the overflow
some different way.

---
Cheers,
Benno