| Message ID | 5e0e8325620395196608a0851f1f6fc9408f6090.1742945534.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Avoid the comma operator | expand |
On Tue, Mar 25, 2025 at 11:32:10PM +0000, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > 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@gmx.de> > --- > 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? Patrick
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@gmx.de> > > > > 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@gmx.de> > > --- > > 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
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 >
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
(Just picking the latest message to reply to, not really quite right) > > > 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; Might be overkill but: #define XXX(index) do { \ if (moff & (0xffUL << ((index) * 8))) { out[outpos++] = moff >> ((index) * 8); i |= 1 << (index); } } while (0) XXX(0); XXX(1); XXX(2); XXX(3); #undef XXX would do the trick. Pick a proper name for XXX of course. Chris
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; *op = i;