Re: [PATCH v2 1/5] qemu/qarray.h: introduce QArray

[Daniel P . Berrangé]

On Wed, Sep 29, 2021 at 07:32:39PM +0200, Christian Schoenebeck wrote:
> On Dienstag, 28. September 2021 18:41:17 CEST Daniel P. Berrangé wrote:
> > On Tue, Sep 28, 2021 at 06:23:23PM +0200, Christian Schoenebeck wrote:
> > > On Dienstag, 28. September 2021 15:04:36 CEST Daniel P. Berrangé wrote:
> > > > On Sun, Aug 22, 2021 at 03:16:46PM +0200, Christian Schoenebeck wrote:
> [...]

> > > Personally I would not mix in this case macros of foreign libraries (glib)
> > > with macros of a local framework (QArray), because if for some reason one
> > > of the two deviate in future in a certain way, you would need to refactor
> > > a whole bunch of user code. By just separating those definitions from day
> > > one, you can avoid such future refactoring work right from the start.
> > 
> > The GLib automatic memory support is explicitly designed to be extendd
> > with support for application specific types. We already do exactly that
> > all over QEMU with many calls to G_DEFINE_AUTOPTR_CLEANUP_FUNC(..) to
> > register functions for free'ing specific types, such that you can
> > use 'g_autoptr' with them.
> 
> Ok, just to make sure that I am not missing something here, because really if 
> there is already something that does the job that I simply haven't seen, then 
> I happily drop this QArray code.

I don't believe there is anything that currently addresses this well.


> But AFAICS this G_DEFINE_AUTOPTR_CLEANUP_FUNC() & g_autoptr concept does not 
> have any notion of "size" or "amount", right?

Correct, all it knows is that there's a data type and an associated
free function.

> So let's say you already have the following type and cleanup function in your 
> existing code:
> 
> typedef struct MyScalar {
>     int a;
>     char *b;
> } MyScalar;
> 
> void myscalar_free(MayScalar *s) {
>     g_free(s->b);
> }
> 
> Then if you want to use G_DEFINE_AUTOPTR_CLEANUP_FUNC() for an array on that 
> scalar type, then you still would need to *manually* write additionally a 
> separate type and cleanup function like:
> 
> typedef struct MyArray {
>     MyScalar *s;
>     int n;
> };
> 
> void myarray_free(MyArray *a) {
>     for (int i = 0; i < a->n; ++i) {
>         myscalar_free(a->s[i]);
>     }
>     g_free(a);
> }
> 
> Plus you have to manually populate that field 'n' after allocation.
> 
> Am I wrong?

Yes and no.  You can of course manually write all these stuff
as you describe, but since we expect the array wrappers to be
needed for more than one type it makes more sense to have
that all done via macros.

Your patch contains a DECLARE_QARRAY_TYPE and DEFINE_QARRAY_TYPE
that provide all this reqiured boilerplate code.  The essential
difference that I'm suggesting is that the array struct type emitted
by the macro is explicitly visible as a concept to calling code such
that it is used directly used with g_autoptr.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|