Re: [PATCH v2 06/10] diff-delta: explicitly mark intentional use of the comma operator

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 26, 2025, 20:33 UTC
Message-ID: <Z+RkmLzciwT6ubIE@nand.local>
In-Reply-To: <ad57203b-8925-4f88-add6-763c2d80afac@gmail.com>

On Wed, Mar 26, 2025 at 10:17:50AM +0000, Phillip Wood wrote:

Show 35 quoted lines

> Hi Johannes
>
> On 26/03/2025 07:20, Johannes Schindelin wrote:
> > Hi Patrick,
> > On Wed, 26 Mar 2025, Patrick Steinhardt wrote:
> > > Hm. I think the end result is even more confusing than before. Why don't
> > > we introduce curly braces here, same as in preceding commits?
> >
> > The interleaved -/+ lines make it admittedly hard to see what I meant.
> > I'll unwind it a bit (presenting only the `moff` part, the same
> > consideration applies to `msize`):
> >
> > 		if (moff & 0x000000ff)
> > 			(void)(out[outpos++] = moff >> 0),  i |= 0x01;
> > 		if (moff & 0x0000ff00)
> > 			(void)(out[outpos++] = moff >> 8),  i |= 0x02;
> > 		if (moff & 0x00ff0000)
> > 			(void)(out[outpos++] = moff >> 16), i |= 0x04;
> > 		if (moff & 0xff000000)
> > 			(void)(out[outpos++] = moff >> 24), i |= 0x08;
> >
> > In this form, it is very obvious (from comparing the right-side half of
> > the lines) that a shifted version of `moff` is appended to `out` and `i`
> > gets a bit set, and the correlation between shift width and the set bit
> > is relatively easy to see and validate (at least my brain has an easy time
> > here, thanks to the alignment and thanks to visual similarity between the
> > non-blank parts).
> >
> > It is admittedly quite a bit harder not to be distracted by the repetitive
> > `(void)(out[...` parts to understand and validate the `if` conditions on
> > the left-hand side,
>
> That makes it pretty unreadable for me. If you're worried about the vertical
> space we could perhaps keep both statements on a single line so that we're
> only adding a single newline for the closing brace rather than two.

My $.02 here would be that the form:

    if (moff & 0x000000ff) {
        out[outpos++] = moff >> 0;
        i |= 0x01;
    }

is the most readable, and fits within the conventions of our CodingGuidelines. I don't really love the idea of writing:

    if (moff & 0x000000ff) {
        out[outpos++] = moff >> 0); i |= 0x01;
    }

, since it looks like a single-line if-statement and is thus tempting to drop the braces, which would make the code incorrect, as 'i |= 0x01' would be executed unconditionally. I suppose you could write

    if (moff & 0x000000ff) {
        out[outpos++] = moff >> 0); i |= 0x01; }

, but that feels even uglier to me ;-).

Thanks, Taylor