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.
Best Wishes
Phillip
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
