Re: [Xen-devel] [PATCH V3] xl: create VFB for PV guest when VNC is specified

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Wei Liu writes ("Re: [Xen-devel] [PATCH V3] xl: create VFB for PV guest when 
VNC is specified"):
> On Tue, Dec 17, 2013 at 11:41:29AM +0000, Ian Campbell wrote:
...
> > In hindsight its main use is probably in making macros only evaluate
> > once, so if it itself did the evaluation it wouldn't be very useful!
> 
> So now the macro becomes:
> 
> #define ARRAY_EXTEND(ptr,count,ret)                                     \
>     do {                                                                \
>         typeof((ptr)) *__ptr = &(ptr);                                  \
>         typeof((count)) *__count = &(count);                            \
>         *__ptr = xrealloc(*__ptr,                                       \
>                           sizeof(**__ptr) * (*__count + 1));            \
>         (ret) = *__ptr + *__count;                                      \
>         (*__count)++;                                                   \
>     } while (0)
> 
> Thoughts?

I think all of this complexity to avoid evaluating the array and count
arguments more than once is unnecessary.  Particularly when they're
both going to be updated.

But if you are doing to do that, you need to give your local variables
different names.  Firstly: names starting __ are reserved for the C
implementation and shouldn't be used for anything at all.  Secondly,
prefixing the names with something like "_" isn't sufficient in the
general case and is the wrong habit.  If you need to define a local
variable in a macro you should decorate it with something related to
the name of the macro.  (Hopefully no-one will invoke the same macro
in one of its own arguments.)

Please make the macro return the new array entry as Ian C suggested.

Finally: IMO the macro should probably initialise the new element
itself.  So it will be more than just "ARRAY_EXTEND" because it will
have some knowledge of the kind of array it is, and might need to be
renamed.  Ian C, what do you think ?

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel