Hi

On Tue, Sep 29, 2020 at 10:23 PM Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 29/09/20 19:55, Marc-André Lureau wrote:
> > My understanding of what you propose is:
> > - ForeignConvert::with_foreign
> > - FromForeign::from_foreign (with implied into_native)
> > And:
> > - ForeignConvert::as_foreign (with the BorrowedPointer/stash-like)
> > - ToForeign::to_foreign + ForeignConvert::as_foreign_mut (which seems
> > wrongly designed in your proposal and unnecessary for now)
>
> Might well be, but how is it wrong?  (I'd like to improve).
>

Why BorrowedMutPointer provides both *const P and *mut P ? The *const P
conversion seems overlapping with BorrowedPointer.

Anyway, the &mut T -> *mut P conversion seems fairly rare to me and
error-prone. You usually give up ownership if you let the foreign function
tweak the P.

In any case, we don't need such conversion for QAPI, for now.


> > I don't have your head, so I find it hard to remember & work with. It>
> uses all possible prefixes: with_, from_, as_, as_mut, to_, and into_.
> > That just blows my mind, sorry :)
>
> Ahah I don't have your head either!  The idea anyway is to reuse
> prefixes that are common in Rust code:
>
> * with_: a constructor that uses something to build a type (think
> Vec::with_capacity) and therefore takes ownership
>


ForeignConvert::with_foreign (const *P -> T) doesn't take ownership.

The Rust reference for this kind of conversion is CStr::from_ptr.


> * as_: a cheap conversion to something, it's cheap because it reuses the
> lifetime (and therefore takes no ownership).  Think Option::as_ref.
>

as_ shouldn't create any object, and is thus unsuitable for a general
rs<->sys conversion function (any).

* from_/to_: a copying and possibly expensive conversion (that you have
> to write the code for).  Because it's copying, it doesn't consume the
> argument (for from_) or self (for to_).
>
>
and that's what glib-rs uses (and CStr).



> * into_: a conversion that consumes the receiver
>
>
That's not used by glib afaik, but we should be able to introduce it for
"mut *P -> T", it's not incompatible with FromPtrFull::from_full.

In general, I like the fact that the conversion traits are associated to T,
and not to P (which can remain a bare pointer, without much associated
methods).

It may well be over the top.
>
> > Then, I don't understand why ForeignConvert should hold both the "const
> > *P -> T" and "&T -> const *P" conversions. Except the common types,
> > what's the relation between the two?
>
> Maybe I'm wrong, but why would you need just one?
>

No I mean they could be on different traits. One could be implemented
without the other. Or else I don't understand why the other conversion
functions would not be in that trait too.


> > Finally, I thought you introduced some differences with the stash
> > design, but in fact I can see that ForeignConvert::Storage works just
> > the way as ToPtr::Storage. So composition should be similar. Only your
> > example code is more repetitive as it doesn't indirectly refer to the
> > trait Storage the same way as glib-rs does (via <T as ToPtr>::Storage).
>
> Yes, that's the main difference.  I removed Storage because I didn't
> want to force any trait on BorrowedPointer's second type argument.  It
> seemed like a generic concept to me.
>

To the cost of some duplication. I like the coupling between the traits
better. If you need a similar tuple/struct elsewhere, it's easy to make
your own.

The Storage type can quickly become quite complicated with QAPI, I would
rather avoid having to repeat it, it would create hideous compiler errors
too.


> The other difference is that Stash is a tuple while BorrowedPointer is a
> struct and has methods to access it.  Stash seems very ugly to use.
>

Yes I agree. Not sure why they made it a bare tuple, laziness perhaps :).


-- 
Marc-André Lureau