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

From: René Scharfe <l.s.r@web.de>
Date: Sep 9, 2020, 19:54 UTC
Message-ID: <0773b560-b456-fc88-42d7-f214246ddd1b@web.de>
In-Reply-To: <20200909191746.GA2514794@coredump.intra.peff.net>

Am 09.09.20 um 21:17 schrieb Jeff King:

Show 8 quoted lines

> On Wed, Sep 09, 2020 at 03:13:46PM -0400, Jeff King wrote:
>
>> 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).  But I
>> also wonder if giving the specific types in the coccinelle rule is
>> buying us anything. If you passed two void pointers or ints or whatever
>> to !oidcmp(), we'd still want to rewrite it as oideq().

Right, using expressions for such a like-for-like transformation is safe and practical in the sense that it won't break correct code, and broken code will be flagged by the compiler.

Show 20 quoted lines

>
> And indeed, just blindly swapping out "struct object_id" for
> "expression" in the coccinelle file (patch below), shows another spot
> that was missed:
>
> diff -u -p a/packfile.c b/packfile.c
> --- a/packfile.c
> +++ b/packfile.c
> @@ -735,7 +735,7 @@ struct packed_git *add_packed_git(const
>  	p->mtime = st.st_mtime;
>  	if (path_len < the_hash_algo->hexsz ||
>  	    get_sha1_hex(path + path_len - the_hash_algo->hexsz, p->hash))
> -		hashclr(p->hash);
> +		oidclr(p);
>  	return p;
>  }
>
>
> Maybe it's worth being looser in our cocci patch definitions. I'm having
> trouble thinking of a downside...

For transformations that change the type as in the example above we should insist on getting the right one, otherwise we might introduce bugs -- like in the example above. p points to a struct packed_git and not to a struct object_id, so this introduces a type mismatch.

We better make sure our semantic patches are safe, otherwise we have to check all conversions very carefully, and then we might be better off doing them manually..

René