Hi Patrick,
On Wed, 26 Mar 2025, Patrick Steinhardt wrote:
> On Tue, Mar 25, 2025 at 11:32:10PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@xxxxxx>
> >
> > The comma operator is a somewhat obscure C feature that is often used by
> > mistake and can even cause unintentional code flow. That is why the
> > `-Wcomma` option of clang was introduced: To identify unintentional uses
> > of the comma operator.
> >
> > Intentional uses include situations where one wants to avoid curly
> > brackets around multiple statements that need to be guarded by a
> > condition. This is the case here, as the repetitive nature of the
> > statements is easier to see for a human reader this way.
> >
> > To mark this usage as intentional, the return value of the statement
> > before the comma needs to be cast to `void`, which we do here.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
> > ---
> > diff-delta.c | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/diff-delta.c b/diff-delta.c
> > index a4faf73829b..a03ba10b2be 100644
> > --- a/diff-delta.c
> > +++ b/diff-delta.c
> > @@ -439,18 +439,18 @@ create_delta(const struct delta_index *index,
> > i = 0x80;
> >
> > if (moff & 0x000000ff)
> > - out[outpos++] = moff >> 0, i |= 0x01;
> > + (void)(out[outpos++] = moff >> 0), i |= 0x01;
> > if (moff & 0x0000ff00)
> > - out[outpos++] = moff >> 8, i |= 0x02;
> > + (void)(out[outpos++] = moff >> 8), i |= 0x02;
> > if (moff & 0x00ff0000)
> > - out[outpos++] = moff >> 16, i |= 0x04;
> > + (void)(out[outpos++] = moff >> 16), i |= 0x04;
> > if (moff & 0xff000000)
> > - out[outpos++] = moff >> 24, i |= 0x08;
> > + (void)(out[outpos++] = moff >> 24), i |= 0x08;
> >
> > if (msize & 0x00ff)
> > - out[outpos++] = msize >> 0, i |= 0x10;
> > + (void)(out[outpos++] = msize >> 0), i |= 0x10;
> > if (msize & 0xff00)
> > - out[outpos++] = msize >> 8, i |= 0x20;
> > + (void)(out[outpos++] = msize >> 8), i |= 0x20;
>
> 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, but thanks to those repetitive parts being identical,
and being only one line between those `if` lines, I can bring my brain to
focus only on the differences of the bitmask and understand and verify
them with relatively little effort.
When I compared this form to the following, the cognitive load to wrap my
head around the code is quite a bit higher there:
if (moff & 0x000000ff) {
out[outpos++] = moff >> 0;
i |= 0x01;
}
if (moff & 0x0000ff00) {
out[outpos++] = moff >> 8;
i |= 0x02;
}
if (moff & 0x00ff0000) {
out[outpos++] = moff >> 16;
i |= 0x04;
}
if (moff & 0xff000000) {
out[outpos++] = moff >> 24;
i |= 0x08;
}
The reason is the visual distance between the near-identical code.
Having said that, I do realize that my brain quite possibly works in
special ways here and that the general preference is to go with the latter
form.
Do you have a strong opinion which form to use?
Ciao,
Johannes
