Note: the word after the subject's subsystem should start with a
lower-case letter.
On Sun, Jan 26, 2025 at 01:56:35PM +0100, Sören Krecker wrote:
> Fix some compiler warnings from msvc in add-patch.c for value truncation
> form 64 bit to 32 bit integers. Change unsigned long to size_t for
> correct variable size on linux and windows.
> Add macro str_to_size_t for converting a string to size_t.
There shouldn't be a need for this macro, we already have `strtoumax()`.
And in case the platform doesn't provide it we know to provide our own
implementation.
> Test if convertion fails with over or underflow.
s/convertion/conversion/
> diff --git a/add-patch.c b/add-patch.c
> index 95c67d8c80..4fb6ae2c4b 100644
> --- a/add-patch.c
> +++ b/add-patch.c
> @@ -322,11 +322,12 @@ static void setup_child_process(struct add_p_state *s,
> }
>
> static int parse_range(const char **p,
> - unsigned long *offset, unsigned long *count)
> + size_t *offset, size_t *count)
> {
> char *pend;
> -
> - *offset = strtoul(*p, &pend, 10);
> + *offset = str_to_size_t(*p, &pend, 10);
> + if (errno == ERANGE)
> + return error(_("Number is too large for this field"));
Error messages should start with a lower-case letter.
> if (pend == *p)
> return -1;
> if (*pend != ',') {
> @@ -334,7 +335,9 @@ static int parse_range(const char **p,
> *p = pend;
> return 0;
> }
> - *count = strtoul(pend + 1, (char **)p, 10);
> + *count = str_to_size_t(pend + 1, (char **)p, 10);
> + if (errno == ERANGE)
> + return error(_("Number is too large for this field"));
Here, too.
> @@ -1066,11 +1071,13 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
>
> /* last hunk simply gets the rest */
> if (header->old_offset != remaining.old_offset)
> - BUG("miscounted old_offset: %lu != %lu",
> - header->old_offset, remaining.old_offset);
> + BUG("miscounted old_offset: %"PRIuMAX" != %"PRIuMAX,
> + (uintmax_t)header->old_offset,
> + (uintmax_t)remaining.old_offset);
> if (header->new_offset != remaining.new_offset)
> - BUG("miscounted new_offset: %lu != %lu",
> - header->new_offset, remaining.new_offset);
> + BUG("miscounted new_offset: %"PRIuMAX" != %"PRIuMAX,
> + (uintmax_t)header->new_offset,
> + (uintmax_t)remaining.new_offset);
> header->old_count = remaining.old_count;
> header->new_count = remaining.new_count;
> hunk->end = end;
I feel like most of the changes are adapting formatting directives like
this. Might be worthwhile to separate into a standalone commit. That'd
also allow the commit message to read less like a list of bullet points
and provide more context, explaining the actual change.
> diff --git a/gettext.h b/gettext.h
> index 484cafa562..d36f5a7ade 100644
> --- a/gettext.h
> +++ b/gettext.h
> @@ -53,7 +53,7 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
> }
>
> static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)
> -const char *Q_(const char *msgid, const char *plu, unsigned long n)
> +const char *Q_(const char *msgid, const char *plu, size_t n)
> {
> if (!git_gettext_enabled)
> return n == 1 ? msgid : plu;
This change feels completely unrelated to all the other changes. It
would probably warrant a new commit.
> diff --git a/git-compat-util.h b/git-compat-util.h
> index e283c46c6f..bb9a6c2bc4 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -292,6 +292,13 @@ static inline int _have_unix_sockets(void)
> #include <sys/sysctl.h>
> #endif
>
> +#if SIZE_MAX == ULONG_MAX
> +#define str_to_size_t strtoul
> +#else
> +#define str_to_size_t strtoull
> +#endif
Hm. A couple of comments:
- The function name doesn't match the schema of function names we
already have. I would rather have expected it to be called something
like `strtouz()` or something like that.
- We tend to avoid using `strtoul()` and friends directly, as they are
really hard to get right. See the implementation of `strtoul_ui()`
for all the checks we do there.
- The way the macro is implemented feels quite fragile.
So I'd propose to adapt the approach a bit and introduce a new function
`strtoumax_ui()`:
static inline int strtoumax_ui(char *const *s, int base, unsigned
uintmax_t max, int *result);
The implementation would mostly follow what we have in `strotul_ui()`.
The `max` parameter here could be used to control the maximum that the
caller expects -- if the parsed integer exceeds it, it would return an
error and set `ERANGE`. If we had such a helper, we can then also
reimplement `strtoul_ui()` on top of that function with a simple call to
`strtoumax_ui(s, base, UINT_MAX, result)`.
This would overall be a lot more flexible than what we currently have.
Patrick
