Sören Krecker <soekkle@xxxxxxxxxx> writes:
> Fix some compiler warings from msvw 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
>
> Signed-off-by: Sören Krecker <soekkle@xxxxxxxxxx>
> ---
> add-patch.c | 44 +++++++++++++++++++++++++-------------------
> gettext.h | 2 +-
> 2 files changed, 26 insertions(+), 20 deletions(-)
> struct hunk_header {
> - unsigned long old_offset, old_count, new_offset, new_count;
> + size_t old_offset, old_count, new_offset, new_count;
These are not "size"s in the traditional sense of what size_t is
(i.e. the number of bytes in a region of memory), but are more or
less proportional to that in that they count in number of lines.
If ulong is sufficient to count number of lines in an incoming
patch, then turning size_t may be excessive---are we sure that we
are not unnecessarily using wider-than-necessary size_t in some
places to hold these values for which ulong is sufficient, causing
compilers to emit unnecessary warning?
IOW, if we have variables of unsigned integer of various sizes, we
_could_ rewrite all of them to use uintmax_t and there won't be
truncation-upon-assignment warnings from a compiler, but such a
rewrite can be pointless. We'd need to find where we stuff these
values, which are originally ulong, to size_t, and see if the use of
size_t is really sensible.
> @@ -321,7 +321,7 @@ 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;
>
> @@ -672,8 +672,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
> */
> const char *p;
> size_t len;
> - unsigned long old_offset = header->old_offset;
> - unsigned long new_offset = header->new_offset;
> + size_t old_offset = header->old_offset;
> + size_t new_offset = header->new_offset;
>
> if (!colored) {
> p = s->plain.buf + header->extra_start;
> @@ -699,12 +699,14 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
> else
> new_offset += delta;
>
> - strbuf_addf(out, "@@ -%lu", old_offset);
> + strbuf_addf(out, "@@ -%" PRIuMAX, (uintmax_t)old_offset);
> if (header->old_count != 1)
> - strbuf_addf(out, ",%lu", header->old_count);
> - strbuf_addf(out, " +%lu", new_offset);
> + strbuf_addf(out, ",%" PRIuMAX,
> + (uintmax_t)header->old_count);
> + strbuf_addf(out, " +%" PRIuMAX, (uintmax_t)new_offset);
> if (header->new_count != 1)
> - strbuf_addf(out, ",%lu", header->new_count);
> + strbuf_addf(out, ",%" PRIuMAX,
> + (uintmax_t)header->new_count);
> strbuf_addstr(out, " @@");
>
> if (len)
So far if we left the types of *_offset and count all ulong, I do
not see anything that causes trunation-upon-assignment.
> @@ -1065,11 +1067,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;
This hunk unfortunately may be needed because this function somehow
decided to use size_t for things like "hunk_index" and "end" when it
was added, even though the surrounding functions it interacts with
all used ulong.
> @@ -1353,9 +1357,10 @@ static void summarize_hunk(struct add_p_state *s, struct hunk *hunk,
> struct strbuf *plain = &s->plain;
> size_t len = out->len, i;
>
> - strbuf_addf(out, " -%lu,%lu +%lu,%lu ",
> - header->old_offset, header->old_count,
> - header->new_offset, header->new_count);
> + strbuf_addf(out,
> + " -%"PRIuMAX",%"PRIuMAX" +%"PRIuMAX",%"PRIuMAX" ",
> + (uintmax_t)header->old_offset, (uintmax_t)header->old_count,
> + (uintmax_t)header->new_offset, (uintmax_t)header->new_count);
> if (out->len - len < SUMMARY_HEADER_WIDTH)
> strbuf_addchars(out, ' ',
> SUMMARY_HEADER_WIDTH + len - out->len);
Again, if we left the types of *_offset and count all ulong, I do
not see anything that causes trunation-upon-assignment.
> @@ -1624,10 +1629,11 @@ static int patch_update_file(struct add_p_state *s,
> else if (0 < response && response <= file_diff->hunk_nr)
> hunk_index = response - 1;
> else
> - err(s, Q_("Sorry, only %d hunk available.",
> - "Sorry, only %d hunks available.",
> - file_diff->hunk_nr),
> - (int)file_diff->hunk_nr);
> + err(s,
> + Q_("Sorry, only %"PRIuMAX" hunk available.",
> + "Sorry, only %"PRIuMAX" hunks available.",
> + (uintmax_t)file_diff->hunk_nr),
> + (uintmax_t)file_diff->hunk_nr);
Again, this hunk may be needed, as the "hunk_nr" uses size_t, which
probably is overly wide.
So, I do not mind too much to adjust the code around hunk_nr,
hunk_alloc and other things that are already size_t (but before
doing so, we probably should see if it makes more sense to use ulong
for these members instead of size_t), hbut I am not sure if it is a
sensible move to change old_offset, count, etc. that count in number
of lines and use ulong (not bytes) to use size_t instead.
size_t _might_ be wider than some other forms of unsigned integers,
but it is not necessarily the widest, so "because on msvc ulong is
merely 32-bit and I want wider integer like everybody else!" is not
a good excuse for such a change (if it were, we'd all coding with
nothing but uintmax_t).
So I dunno.
> 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;
