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

[Wei Liu <wei.liu2@citrix.com>]

On Tue, Dec 17, 2013 at 03:02:07PM +0000, Ian Jackson wrote:
> 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.
> 

OK.

> 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 ?
> 

Huh? Elements are initialized by different funcitons. You mean I need to
add extra argument to the macro to pass in the initilization function?

Wei.

> Ian.