Re: [PATCH v6 04/14] block/amend: separate amend and create options for qemu-img

[Maxim Levitsky <mlevitsk@redhat.com>]

On Fri, 2020-05-15 at 12:24 -0500, Eric Blake wrote:
> On 5/15/20 1:22 AM, Max Reitz wrote:
> 
> > > > 
> > > > > +        QCOW_COMMON_OPTIONS,
> > > > > +        { /* end of list */ }
> > > 
> > > ...the intended usage is to use the macro name followed by a comma, so
> > > including a trailing comma in the macro itself would lead to a syntax
> > > error.
> > 
> > But why is that the indended usage?  Is there something in our coding
> > style that forbids macros that don’t allow a separator to be placed
> > after them?
> 
> If we have more than one such macro, it is easier to write and indent 
> (especially when using your editor's ability to decipher enough syntax 
> to suggest how to indent):
> 
> myarray = {
>    COMMON_ELEMENTS,
>    MORE_ELEMENTS,
>    { /* end of list */ }
> };
> 
> than it is:
> 
> myarray = {
>    COMMON_ELEMENTS
>    MORE_ELEMENTS
>    { /* end of list */ }
> };
> 
> which in turn implies that it is better to NOT stick a trailing comma in 
> the macro itself.  Similarly, for macros intended to replace statements, 
> we tend to avoid the trailing ; in the macro itself, because it is 
> easier to read:
> 
> {
>    code;
>    MACRO();
>    more code;
> }
> 
> than it is:
> 
> {
>    code;
>    MACRO()
>    more code;
> }
> 

100% agree with that.


Here something a bit off-topic, but something that I find a bit amusing and is somewhat related to
hiding punctuation in macros:

I once wasted about hour of my life trying to understand why kernel ABI macro I added for a backport
didn't work as intended.
(This was a macro we are supposed to wrap each new struct field in it to
inform to the ABI checker
that it is OK).

I was almost ready to poke my eyes out as I were comparing what I wrote to what is present in
few more places that use that macro, till I finally understood that the macro expects you to
not stick ';' after it. It does compile fine with an extra ';', since it is just an empty statement,
but this was tripping some regular expression in the ABI checker script or something.
(It didn't give me any meaningful error).

Back to topic, I'll rebase this patch, as I always do prior to sending
a new patch series.


Best regards,
	Maxim Levitsky