Re: [PATCH v2] Avoid address_space_rw() with a constant is_write argument

[Peter Maydell <peter.maydell@linaro.org>]

On Tue, 18 Feb 2020 at 13:33, Eric Blake <eblake@redhat.com> wrote:
>
> On 2/18/20 6:56 AM, Philippe Mathieu-Daudé wrote:
>
> >> +++ b/scripts/coccinelle/as_rw_const.cocci
> >> @@ -0,0 +1,30 @@
> >> +// Avoid uses of address_space_rw() with a constant is_write argument.
> >> +// Usage:
> >> +//  spatch --sp-file as-rw-const.spatch --dir . --in-place
> >
> > Nitpick, script is now scripts/coccinelle/as_rw_const.cocci.
> >
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >
> >> +
> >> +@@
> >> +expression E1, E2, E3, E4, E5;
> >> +symbol false;
> >> +@@
> >> +
> >> +- address_space_rw(E1, E2, E3, E4, E5, false)
> >> ++ address_space_read(E1, E2, E3, E4, E5)
> >> +@@
> >> +expression E1, E2, E3, E4, E5;
> >> +@@
> >> +
> >> +- address_space_rw(E1, E2, E3, E4, E5, 0)
> >> ++ address_space_read(E1, E2, E3, E4, E5)
>
> This feels a bit redundant.  Doesn't coccinelle have enough smarts about
> isomorphisms (such as 0 == false, 1 == true) that it could get by with
> one @@ hunk instead of 2, if we come up with the right way to represent
> any isomorphism to a constant value?  But admittedly, I don't know what
> that representation would actually be, and your verbose patch works even
> if it is not the most concise possible.  So don't let my remarks hold
> this patch up.

My experience with Coccinelle has generally been that trying
to make semantic patches smaller and less redundant is futile
and a massive timesink. In this case as far as I can tell
Coccinelle has no idea at all about the existence of the 'bool'
type and that 'true' and 'false' are equivalent to 1 and 0.
Thus the 'symbol' declaration, otherwise it thinks that 'false'
is a random semantic identifier and doesn't look for a literal
match of it.

thanks
-- PMM