Subject: Re: [PATCH] git: use U to denote unsigned to prevent UB

From: Jonathan Nieder <[email-masked]>
Date: Thu, 2 Jan 2025 16:43:43 +0100

Hi,

Seija Kijin wrote:

> 1 << can be UB if 1 ends up overflowing and
> being assigned to an unsigned int or long.
>
> Signed-off-by: Seija Kijin <[email-masked]>
> ---
>  builtin/checkout.c     |  2 +-
>  builtin/merge-tree.c   |  4 ++--
>  builtin/receive-pack.c |  2 +-
>  color.c                |  4 ++--
>  delta-islands.c        |  2 +-
>  diff-delta.c           |  2 +-
>  diff.c                 |  2 +-
>  help.c                 |  2 +-
>  imap-send.c            |  2 +-
>  merge-ort.c            | 18 +++++++++---------
>  xdiff/xhistogram.c     |  2 +-
>  xdiff/xprepare.c       |  4 ++--
>  12 files changed, 23 insertions(+), 23 deletions(-)

That said, most of these don't overflow, so it's not obvious this
results in higher quality or more readable code than before the patch.

By "not obvious" I don't mean that it _doesn't_, by the way, but just
that we don't have enough information to evaluate it here.  What
motivated writing this patch?  Is there a style guideline about it
that will remind us not to backslide in the future, for example?  Or
is there a tool that notices?  Was there an example you ran into that
led you to look for more examples?

This kind of information about context will make it easier for other
in the project to ensure the patch does what it intends, and even more
importantly, to see if there are additional checks to add or other
instances that also need updating.

Thanks and hope that helps,
Jonathan