Re: [PATCH 2/2] rust: alloc: add `Vec::dec_len`

[Tamir Duberstein]

On Mon, Mar 17, 2025 at 10:42 AM Benno Lossin <benno.lossin@xxxxxxxxx> wrote:
>
> On Mon Mar 17, 2025 at 1:59 PM CET, Alice Ryhl wrote:
> > On Mon, Mar 17, 2025 at 11:47:50AM +0000, Alice Ryhl wrote:
> >> On Mon, Mar 17, 2025 at 07:34:44AM -0400, Tamir Duberstein wrote:
> >> > On Mon, Mar 17, 2025 at 6:04 AM Benno Lossin <benno.lossin@xxxxxxxxx> wrote:
> >> > >
> >> > > On Sun Mar 16, 2025 at 11:32 PM CET, Tamir Duberstein wrote:
> >> > > > Add `Vec::dec_len` that reduces the length of the receiver. This method
> >> > > > is intended to be used from methods that remove elements from `Vec` such
> >> > > > as `truncate`, `pop`, `remove`, and others. This method is intentionally
> >> > > > not `pub`.
> >> > >
> >> > > I think it should be `pub`. Otherwise we're loosing functionality
> >> > > compared to now. If one decides to give the raw pointer to some C API
> >> > > that takes ownership of the pointer, then I want them to be able to call
> >> > > `dec_len` manually.
> >> >
> >> > This is premature. It is trivial to make this function pub when the need arises.
> >>
> >> Normally I'd agree with Benno, but in this case I think having it
> >> private is preferable. The function is safe, so it's too easy for
> >> end-users to confuse it with truncate.
> >
> > Thinking more about this ... I think we should have `set_len` and
> > `inc_len` instead. That way, both methods are unsafe so people will not
> > accidentally use `set_len` when they meant to use `truncate`.
>
> I agree for this on the public API. The way I usually saw `set_len`
> being used for decrementing was truncation without dropping the old
> values. And that is going to be `vec.dec_len(vec.len())` with the
> current design. `vec.set_len(0);` is much clearer in that respect.
>
> But for the internals, I'd say that `dec_len` is nicer, so for `pop` one
> would then use `self.dec_len(1)`.
>
> How about we keep `set_len` and make `dec_len` a private, safe helper?

This discussion is _way_ too speculative for my taste. If you'd like
to do this kind of thing, I'm happy to drop this patch or the series.
I'm not comfortable adding API whose usage I haven't seen and don't
understand.