"AreaZR via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Seija Kijin <doremylover123@xxxxxxxxx>
>
> 1 << can be UB if 1 ends up overflowing and
> being assigned to an unsigned int or long.
* Spell out what you meant by "UB".
* "1 ends up overflowing"? One is one; as long as you have two
bits, it won't overflow. This needs rewriting.
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 5e5afa0f267..a636e71e05c 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -223,7 +223,7 @@ static int check_stages(unsigned stages, const struct cache_entry *ce, int pos)
> ce = the_repository->index->cache[pos];
> if (strcmp(name, ce->name))
> break;
> - seen |= (1 << ce_stage(ce));
> + seen |= (1U << ce_stage(ce));
Here "seen" is "unsigned" initialized to 0. Matching the type of
the value that is assigned with an explicit U does make sense, but
as Jonathan already pointed out elsewhere, as ce_stage() cannot be
more than 3, this would never overflow.
> diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> index c5ed472967a..d0104dfa0c7 100644
> --- a/builtin/merge-tree.c
> +++ b/builtin/merge-tree.c
> @@ -270,13 +270,13 @@ static void unresolved(const struct traverse_info *info, struct name_entry n[3])
> unsigned dirmask = 0, mask = 0;
>
> for (i = 0; i < 3; i++) {
> - mask |= (1 << i);
> + mask |= (1U << i);
Ditto.
> /*
> * Treat missing entries as directories so that we return
> * after unresolved_directory has handled this.
> */
> if (!n[i].mode || S_ISDIR(n[i].mode))
> - dirmask |= (1 << i);
> + dirmask |= (1U << i);
Ditto.
There are a few instances that left shifts by 31-bit, which requires
(1 * 2 ** 31) to be representable in the result type of signed int
if we want to avoid an undefined behaviour,, but even if your signed
int were wider than 32-bit, it is a good hygiene to write your bit
shift as (1U << shift_count).
So I am not opposed to these changes. The guiding principle should
probably be "bit patterns should by unsigned by default, unless you
have a strong and valid reason to use signed" (and the only single
plausible reason being when you take advantage of fact that the sign
bit is propagated if you shift right); as most of the changed code
paths do deal with a signed result that is representable and does
not risk any undefined behaviour, it is an inappropriate rationale
to justify this particular patch, I would think.
