Vladimir Sementsov-Ogievskiy <vsement...@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 <arm...@redhat.com>
>>> Reviewed-by: Eric Blake <ebl...@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.
