Re: [Qemu-devel] [PATCH v9 15/37] qom: Swap 'name' next to visitor in ObjectPropertyAccessor

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 01/20/2016 11:49 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> Similar to the previous patch, it's nice to have all functions
>>> in the tree that involve a visitor and a name for conversion to
>>> or from QAPI to consistently stick the 'name' parameter next
>>> to the Visitor parameter.
>>>
>>> Done by manually changing include/qom/object.h and qom/object.c,
>>> then running this Coccinelle script and touching up the fallout
>>> (Coccinelle insisted on adding some trailing whitespace).
>>>
>>>     @ rule1 @
>>>     identifier fn;
>>>     type Object, Visitor, Error;
>>>     identifier obj, v, opaque, name, errp;
>>>     @@
>>>      void fn
>>>     - (Object *obj, Visitor *v, void *opaque, const char *name,
>>>     + (Object *obj, Visitor *v, const char *name, void *opaque,
>>>        Error **errp) { ... }
>> 
>> I think we want to match void functions with exactly these parameter
>> types.  The parameter names don't matter.
>
> The parameter names shouldn't matter; the 'identifier obj' should have
> been enough to make 'obj' a metavariable matching any actual parameter name.
>
>> 
>> However, the actual match is looser!  For instance, it also matches
>> 
>>     void foo(int *pi, unsigned *pu, void *vp, const char *cp, double **dpp)
>>     {
>>     }
>
> Uggh. My intent was to match exactly 'Object *' and 'Visitor *' as the
> first two types, where 'int *' and 'unsigned *' are NOT matches.  But I
> don't know Coccinelle well enough to make that blatantly obvious (is my
> declaration of 'type Object' not correct?).

'type Object' makes 'Object' a metavariable matching any C type.

>> This could mess up unrelated function.  I could double-check it doesn't,
>> but I'd rather have a narrower match instead.  Can't give one offhand,
>> though.  Ideas?
>
> Is 'typedef' better than 'type' for constraining the type of the first
> two arguments?

Matches any C typedef name.  Less wrong, but still wrong :)

>                 Or does Coccinelle do literal matches on anything you
> don't pre-declare, as in:

Yes, but...

>      @ rule1 @
>      identifier fn;
>      identifier obj, v, opaque, name, errp;
>      @@
>       void fn
>      - (Object *obj, Visitor *v, void *opaque, const char *name,
>      + (Object *obj, Visitor *v, const char *name, void *opaque,
>         Error **errp) { ... }

... when I try that, spatch throws a parse error.

> Fortunately, a manual inspection of the results (which I had to do
> anyways due to spacing issues) didn't spot any incorrect swaps.
>
> At this point, I don't know that re-writing Coccinelle will be worth the
> hassle (nothing else needs to be rewritten).

I'd normally take up the challenge to wrestle Coccinelle, but I think
our time is better spent elsewhere right now.  Let's amend the commit
message instead: describe the semantic patch's shortcomings, and your
manual checking:

    This semantic patch is actually far too lose: it matches *any* type,
    not just Object, Visitor, Error.  However, I can't figure out how to
    make Coccinelle match Object, Visitor, Error exactly.  Simply
    deleting the 'type Object, Visitor, Error;' line results in a parse
    error.  I reviewed the resulting patch carefully to verify there are
    no unwanted matches.

>>>     @@
>>>     identifier rule1.fn;
>>>     expression obj, v, opaque, name, errp;
>>>     @@
>>>      fn(obj, v,
>>>     -   opaque, name,
>>>     +   name, opaque,
>>>         errp)
>> 
>> The rule1.fn restricts the match to functions changed by the previous
>> rule.  Good.
>> 
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>