Re: [PATCH v2] rust: alloc: fix dangling pointer in VecExt<T>::reserve()

[Danilo Krummrich <dakr@redhat.com>]

On Tue, May 07, 2024 at 12:24:56AM -0300, Wedson Almeida Filho wrote:
> On Tue, 30 Apr 2024 at 19:04, Danilo Krummrich <dakr@redhat.com> wrote:
> >
> > On Tue, Apr 30, 2024 at 04:25:44PM -0300, Wedson Almeida Filho wrote:
> > > On Tue, 30 Apr 2024 at 09:13, Danilo Krummrich <dakr@redhat.com> wrote:
> > >
> > > >          let (ptr, len, cap) = destructure(self);
> > > >
> > > > +        // We need to make sure that ptr is either NULL or comes from a previous call to
> > > > +        // `krealloc_aligned`. A `Vec<T>`'s `ptr` value is not guaranteed to be NULL and might be
> > > > +        // dangling after being created with `Vec::new`. Instead, we can rely on `Vec<T>'s capacity
> > > > +        // to be zero if no memory has been allocated yet.
> > > > +        let ptr = match cap {
> > > > +            0 => ptr::null_mut(),
> > > > +            _ => ptr,
> > > > +        };
> > > > +
> > >
> > > nit: why did you choose to use a match here?
> >
> > I felt like it reads nicely.
> >
> > > I don't think C
> > > programmers would use a switch to determine if a value is zero or
> > > non-zero.
> >
> > Well, I think for writing Rust code it doesn't matter too much what C
> > programmers would do? ;-)
> >
> > What is idiomatic in this case?
> 
> The idiomatic way is to use `if`. Even in cases where a match was
> required (e.g., when checking an enum) and only two options are ever
> used, the languages has the matches!
> (https://doc.rust-lang.org/std/macro.matches.html) macro so you'd do:
> 
> if matches!(a, A::X(_)) {
> } else {
> }
> 
> instead of
> 
> match a {
>     A::X(_) => {
>     }
>     _ => {
>     }
> }
> 
> They even changed the language with if-let constructs
> (https://rust-lang.github.io/rfcs/0160-if-let.html) so that we'd do
> 
> if let A::X(_) = a {
> } else {
> }
> 
> instead of the match above. Here's the first paragraph for the
> justification for this feature:
> 
> "Introduce a new if let PAT = EXPR { BODY } construct. This allows for
> refutable pattern matching without the syntactic and semantic overhead
> of a full match, and without the corresponding extra rightward drift.
> Informally this is known as an “if-let statement”."
> 

Very valuable information - thanks for sharing!

This is what I was potentially looking for when I was asking "Is there any
benefit using an if here, or is that your personal preference?" in v3.

> So the language designers and std library implementers provide a macro
> and a language construct to avoid using matches when needed, but
> you're going in the opposite direction where you don't need it but
> want to use it anyway.

Please let me clarify this:

It's not really that I want to use it or insist on it. I really just chose it
because I felt like it reads nicely. I did not feel like there is an actual
request for change. Especially, since you phrased it as "nit: why did you choose
to use a match here?", which is a question and even attenuated by the "nit".

When you mentioned in v3 that you "still think this should be an `if`", I
noticed that you might actually want this to be changed and I wondered why.

Me asking "Is there any benefit using an if here, or is that your personal
preference?" wasn't meant to read as if I wouldn't want to change it, but honest
interest in why it's more than just a question and more than a "nit".

Please also see my reply [1] to Miguel.

> 
> Please change this.

Sure, I sent out another patch changing this, since v3 has been merged already [2].
Feel free to melt this one into the original one though.

[1] https://lore.kernel.org/rust-for-linux/b57dde93-06db-405b-ab94-864779c76010@redhat.com/
[2] https://lore.kernel.org/rust-for-linux/CANiq72nSQAE7c8ypWQA__BMoaBYqiEjXvHpuzn-=CsCky9uu7A@mail.gmail.com/

> 
> Thanks,
> -Wedson
>