Re: [PATCH] blame.c: replace instance of !oidcmp for oideq

From: René Scharfe <l.s.r@web.de>
Date: Sep 9, 2020, 20:43 UTC
Message-ID: <84ce64d0-996a-33f6-53be-e47120da81a7@web.de>
In-Reply-To: <20200909191345.GA2511547@coredump.intra.peff.net>

Am 09.09.20 um 21:13 schrieb Jeff King:

Show 31 quoted lines

> On Wed, Sep 09, 2020 at 08:00:57AM -0600, Edmundo Carmona Antoranz wrote:
>
>> On Wed, Sep 9, 2020 at 3:11 AM Jeff King <peff@peff.net> wrote:
>>>
>>> Yeah, it looks obviously correct. I am puzzled why "make coccicheck"
>>> doesn't find this, though. +cc René, as my favorite target for
>>> coccinelle nerd-snipes. :)
>>>
>>
>> I added this to contrib/coccinelle/object_id.cocci in v2.27.0
>>
>> @@
>> identifier f != oideq;
>> expression E1, E2;
>> @@
>> - !oidcmp(E1, E2)
>> + oideq(E1, E2)
>>
>> And it found it:
>
> Interesting. The existing rule is:
>
>   struct object_id *OIDPTR1;
>   struct object_id *OIDPTR2;
>   @@
>   - oidcmp(OIDPTR1, OIDPTR2) == 0
>   + oideq(OIDPTR1, OIDPTR2)
>
> The "== 0" part looks like it might be significant, but it's not.
> Coccinelle knows that "!foo" is the same as "foo == 0" (and you can
> confirm by tweaking it).

It is significant in the sense that "x == 0" in the semantic patch also matches "!x" in the code, but "!x" in the semantic patch doesn't match "x == 0". That's because coccinelle has this isomorphism built in (in /usr/lib/coccinelle/standard.iso on my machine):

Expression
@ not_int1 @
int X;
@@
 !X => X == 0

It's a one-way isomorphism (i.e. a rule that says that certain expressions have the same meaning). So we should use "x == 0" over "!x" in semantic patches to cover both cases.

Show 24 quoted lines

> So the relevant part is probably that our existing rule specifies the
> exact type, whereas your rule allows any expression.
>
> And indeed, if I do this, it works:
>
> diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci
> index ddf4f22bd7..62a6cee0eb 100644
> --- a/contrib/coccinelle/object_id.cocci
> +++ b/contrib/coccinelle/object_id.cocci
> @@ -55,8 +55,8 @@ struct object_id OID;
>  + oidcmp(&OID, OIDPTR)
>
>  @@
> -struct object_id *OIDPTR1;
> -struct object_id *OIDPTR2;
> +expression OIDPTR1;
> +expression OIDPTR2;
>  @@
>  - oidcmp(OIDPTR1, OIDPTR2) == 0
>  + oideq(OIDPTR1, OIDPTR2)
>
> Which really _seems_ like a bug in coccinelle, unless I am missing
> something. Because both of those parameters look like object_id pointers
> (and the compiler would be complaining if it were not the case).

Yes, seems it looks like coccinelle gives up trying to determine the type of these things.

And while this one here matches the example in blame.c:

@@
expression A, B;
@@
- 0 == oidcmp(A, B)
+ oideq(A, B)

... and this one does as well:

@@
expression A, B;
@@
- !oidcmp
+ oideq
  (A, B)

... the following one doesn't:

@@
expression A, B;
@@
- 0 == oidcmp
+ oideq
  (A, B)

... and neither does this one:

@@
expression A, B;
@@
- oidcmp
+ oideq
  (A, B)
- == 0

So it helps to try some variants in the hope to bypass some of the restrictions/bugs/misunderstandings. O_o

René