Re: [PATCH] mergesort: avoid left shift overflow

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 17, 2021, 23:31 UTC
Message-ID: <nycvar.QRO.7.76.6.2111180026580.11028@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqr1bf2l83.fsf@gitster.g>

Hi Junio,

On Wed, 17 Nov 2021, Junio C Hamano wrote:

Show 35 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> diff --git a/mergesort.c b/mergesort.c
> >> index 6216835566..bd9c6ef8ee 100644
> >> --- a/mergesort.c
> >> +++ b/mergesort.c
> >> @@ -63,7 +63,7 @@ void *llist_mergesort(void *list,
> >>  		void *next = get_next_fn(list);
> >>  		if (next)
> >>  			set_next_fn(list, NULL);
> >> -		for (i = 0; n & (1 << i); i++)
> >> +		for (i = 0; n & ((size_t)1 << i); i++)
> >
> > I was a bit concerned about the operator precedence (some of which I
> > remember by heart, some not), but according to
> > https://en.cppreference.com/w/c/language/operator_precedence the cast has
> > a higher precedence than the shift operator.
> >
> > I would have preferred an extra pair of parentheses around `(size_t)1` so
> > that I (and other readers) do not have to remember or look up the operator
> > precedence, but it _is_ correct.
>
> Interesting.
>
> I do not quite see the need for it myself, but if we wanted to, we
> can smoke them out with this, I think.
>
> 	$ cat >contrib/coccinelle/cast.cocci <<-\EOF
> 	@@
> 	type T;
> 	expression V, C;
> 	@@
> 	-(T) V << C
> 	+((T) V) << C
> 	EOF

Cute.

However, I am not a fan of fixing what ain't broken (the many refactorings in v2.34.0 did cause quite some fall-out work in git-for-windows/git and microsoft/git, after all, and at least _I_ do not yet see much benefit to show for that price we paid for those refactorings).

And the primary benefit of enclosing the left operand in parentheses is to make reviews easier, so I would prefer to leave existing (read: _already-reviewed_) code alone.

Thanks, Dscho

Show 14 quoted lines

> 	$ make contrib/coccinelle/cast.cocci.patch
> 	$ git apply --stat contrib/coccinelle/cast.cocci.patch
>          compat/mingw.c     |    2 +-
>          compat/mingw.c     |    2 +-
>          ewah/bitmap.c      |    2 +-
>          ewah/ewok_rlw.h    |    6 +++---
>          ewah/ewah_bitmap.c |    8 ++++----
>          ewah/ewok_rlw.h    |    6 +++---
>          ppc/sha1.c         |    2 +-
>          wrapper.c          |    2 +-
>          8 files changed, 15 insertions(+), 15 deletions(-)
>
>
>