Re: [PATCH v3 1/4] add-patch: Fix type conversion warnings from msvc
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Jan 27, 2025, 07:26 UTC
- Message-ID
- <Z5c1EIXi7nsB2kJe@pks.im>
- In-Reply-To
- <20250126125638.3089-2-soekkle@freenet.de>
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/
Show 17 quoted lines
> 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.
Show 11 quoted lines
> 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.
Show 18 quoted lines
> @@ -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.
Show 13 quoted lines
> 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.
Show 13 quoted lines
> 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