Re: [PATCH] git: use U to denote unsigned to prevent UB
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Jan 2, 2025, 15:43 UTC
- Message-ID
- <Z3a0LzChuIzmr7jw@google.com>
- In-Reply-To
- <pull.1849.git.git.1734488549111.gitgitgadget@gmail.com>
Hi,
Seija Kijin wrote:
Show 18 quoted lines
> 1 << can be UB if 1 ends up overflowing and > being assigned to an unsigned int or long. > > Signed-off-by: Seija Kijin <doremylover123@gmail.com> > --- > 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