On Wed, Mar 26, 2025 at 10:17:50AM +0000, Phillip Wood wrote:
> 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
