Re: [PATCH v1] Travis: also test on 32-bit Linux

From: René Scharfe <l.s.r@web.de>
Date: Mar 10, 2017, 18:31 UTC
Message-ID: <3a1cd5cf-cf13-7ef1-7b47-d4b083b64d11@web.de>
In-Reply-To: <20170310175719.cxol7d262n2rlcb2@sigill.intra.peff.net>

Am 10.03.2017 um 18:57 schrieb Jeff King:

Show 10 quoted lines

> On Fri, Mar 10, 2017 at 05:20:13PM +0100, René Scharfe wrote:
>
>>> I think this misses the other two cases: (*dst, src) and (*dst, *src).
>>
>> ... and that's why I left them out.  You can't get dst vs. *dst wrong with
>> structs (at least not without the compiler complaining); only safe
>> transformations are included in this round.
>
> I don't think the transformation could be wrong without the original
> code being wrong.

Avoiding to introduce bugs with automatic transformations is essential, indeed -- if we're not careful here we'd be better off keeping the original code.

Show 12 quoted lines

> I'm also not sure why mine would be unsafe and yours would be safe. It
> seems like the other way around, because mine will do:
>
>   *dst = ...
>
> which would fail unless "dst" is a pointer. Whereas in yours, we end up
> with:
>
>   dst = ...
>
> and somebody mistaking pointer assignment for a struct copy would not
> notice.

If dst is a struct then having *dst on the left-hand side results in a compiler error -- you can't get that one wrong. If it's a pointer then both dst and *dst can be valid (pointer assignment vs. content copy), so there is the possibility of making a mistake without the compiler noticing.

> But either way, I don't think we can have a rule like "you can use
> struct assignment only if you don't have a pointer for the destination".
> That's too arcane to expect developers and reviewers to follow. We
> should either allow struct assignment or not.

With an automatic transformation in place it's more like "you can't use memcpy() in this case any more as it gets turned into an assignment with the next cocci patch". I think we shouldn't be that restrictive for pointers as destinations (yet?).

René