Hi On Tue, Sep 29, 2020 at 10:23 PM Paolo Bonzini <pbonz...@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
