Re: [PATCH v2 26/44] qom: Put name parameter before value / visitor parameter

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 03.07.2020 21:05, Vladimir Sementsov-Ogievskiy wrote:
>> 02.07.2020 18:49, Markus Armbruster wrote:
>>> The object_property_set_FOO() setters take property name and value in
>>> an unusual order:
>>>
>>>      void object_property_set_FOO(Object *obj, FOO_TYPE value,
>>>                                   const char *name, Error **errp)
>>>
>>> Having to pass value before name feels grating.  Swap them.
>>>
>>> Same for object_property_set(), object_property_get(), and
>>> object_property_parse().
>>>
>>> Convert callers with this Coccinelle script:
>>>
>>>      @@
>>>      identifier fun = {object_property_get, object_property_parse, 
>>> object_property_set_str, object_property_set_link, 
>>> object_property_set_bool, object_property_set_int, 
>>> object_property_set_uint, object_property_set, object_property_set_qobject};
>>>      expression obj, v, name, errp;
>>>      @@
>>>      -    fun(obj, v, name, errp)
>>>      +    fun(obj, name, v, errp)
>>
>> I'd suggest
>>
>> @@
>> identifier fun = {object_property_get, object_property_parse, 
>> object_property_set_str, object_property_set_link, object_property_set_bool, 
>> object_property_set_int, object_property_set_uint, object_property_set, 
>> object_property_set_qobject};
>> parameter obj, v, name, errp;
>> @@
>> -    fun(obj, v, name, errp)
>> +    fun(obj, name, v, errp)
>>       {...}
>>
>>
>> in addition, to not
>
> do it by hand I mean

Updating the function comments remains manual, as is tweaking line
breaks for saner diffs.  Not sure the additional automation is worth the
trouble here.

>>> Chokes on hw/arm/musicpal.c's lcd_refresh() with the unhelpful error
>>> message "no position information".  Convert that one manually.
>>>
>>> Fails to convert hw/arm/armsse.c, because Coccinelle gets confused by
>>> ARMSSE being used both as typedef and function-like macro there.
>>> Convert manually.
>>>
>>> Fails to convert hw/rx/rx-gdbsim.c, because Coccinelle gets confused
>>> by RXCPU being used both as typedef and function-like macro there.
>>> Convert manually.  The other files using RXCPU that way don't need
>>> conversion.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> The change should be safe, as compiler will complain if something is not 
>> updated correspondingly. The only exclusion are object_property_parse and 
>> object_property_set_str, where both key and value are strings. Check them 
>> presizely looking at
>>
>>   vimdiff <(git grep object_property_parse HEAD^ | sed 's/HEAD\^://') <(git 
>> grep object_property_parse)
>>
>> and
>>
>>   vimdiff <(git grep -A 1 object_property_set_str HEAD^ | sed 's/HEAD\^://') 
>> <(git grep -A 1 object_property_set_str)
>>
>> seems everything is updated.
>>
>> Also, looked through manual changes for hw/arm/musicpal.c, hw/arm/armsse.c 
>> and hw/rx/rx-gdbsim.c. Seems correct..
>>
>>
>>> ---
>>>   include/hw/audio/pcspk.h                 |   2 +-
>>>   include/qom/object.h                     |  45 ++++-----
>>>   include/qom/qom-qobject.h                |   7 +-
>>>   backends/cryptodev.c                     |   2 +-
>>
>> [..]
>>
>>>   static void property_release_tm(Object *obj, const char *name,
>>> @@ -2384,10 +2375,8 @@ static void property_set_uint8_ptr(Object *obj, 
>>> Visitor *v, const char *name,
>>>   {
>>>       uint8_t *field = opaque;
>>>       uint8_t value;
>>> -    Error *local_err = NULL;
>>
>> This (and some more) chunks are obviously from some another patch..

Looks like a rebase went awry.  I'll regenerate the patch.  Thanks!

>>> -    if (!visit_type_uint8(v, name, &value, &local_err)) {
>>> -        error_propagate(errp, local_err);
>>> +    if (!visit_type_uint8(v, name, &value, errp)) {
>>>           return;
>>>       }
>>
>>
>> To find problems like this, I'm trying to rerun your cocci-script, but I 
>> don't know how exactly do you run it.
>>
>> I've tried --use-gitgrep, but it doesn't find some files.
>>
>> I've tried
>> git grep -l 
>> 'object_property_get\|object_property_parse\|object_property_set_str\|object_property_set_link\|object_property_set_bool\|object_property_set_int\|object_property_set_uint\|object_property_set\|object_property_set_qobject'
>>  | xargs spatch script.cocci --macro-file scripts/cocci-macro-file.h 
>> --in-place --no-show-diff
>>
>> Still, have not updated target/arm/monitor.c, strange..

When that happens, Running spatch one more time works sometimes.

Alternatively, you can run it once per file, like this:

    f=; for i in  ...
    do
        spatch --sp-file object_property_set.cocci --macro-file 
scripts/cocci-macro-file.h ... $i || f="$f $i"
    done
    [ "$f" ] && echo "*** FAILED:$f"
    [ -z "$f" ]

Mitigates another issue I have with spatch: it occasionally runs amok
eating up all my RAM.  Seems to be much less likely when I feed it just
one file at a time.

Mind, just because this helps with my version of spatch doesn't mean
it'll help (or even work) with yours.

>> Another fact, which makes it hard to check the patch is a lot of manual 
>> wrapping/indenting updates.. Hmm, I need some tool or script to make it 
>> simple to compare commits ignoring wrapping and indentation differences.

Try git-diff --word-diff=color for visual inspection, =porcelain for
feeding to a script.