threads / rfc / 14278

RFC patchFix some warnings (on cygwin) to allow -Werror

Subject: [PATCH/RFC] Fix some warnings (on cygwin) to allow -Werror

## tl;dr

7 messages between Jul 3, 2008 and Jul 8, 2008. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Ramsay Jones· Jul 3, 2008, 15:52 UTC · lore
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---

Despite the subject line, this should be useful on any system for which uint32_t is defined to be unsigned long rather than unsigned int. (and where the return type of htonl() is similarly defined).

Note that this was built on the v1.5.6 tarball, so will probably not apply cleanly to the git repo! (hey, it's been more than a few days). Hence, the RFC in the subject line. (is there a better TLA for "so old it won't apply" ;-)

Also, note that git-compat-compat-util.h contains an #ifdef'ed definition of PRIuMAX despite an unconditional #include of inttypes.h; this seems a bit odd. This seems to imply a system with a pre-standard header file, missing the PRIuMAX #define. If so, then similar definitions for PRIu32 and PRIx32 may need to be added.

ATB, Ramsay

 builtin-fast-export.c    |    4 ++--
 builtin-fetch-pack.c     |    3 ++-
 builtin-pack-objects.c   |   15 ++++++++++-----
 builtin-unpack-objects.c |    3 ++-
 index-pack.c             |    6 ++++--
 pack-check.c             |    4 ++--
 receive-pack.c           |    3 ++-
 sha1_file.c              |   12 ++++++------
 show-index.c             |    3 ++-
 9 files changed, 32 insertions(+), 21 deletions(-)
Show changes to 9 files +32 −21

builtin-fast-export.c, builtin-fetch-pack.c, builtin-pack-objects.c, builtin-unpack-objects.c, index-pack.c, pack-check.c, receive-pack.c, sha1_file.c, show-index.c

diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index d0a462f..3500ff0 100644
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -102,7 +102,7 @@ static void handle_object(const unsigned char *sha1)
 
 	mark_object(object);
 
-	printf("blob\nmark :%d\ndata %lu\n", last_idnum, size);
+	printf("blob\nmark :%"PRIu32"\ndata %lu\n", last_idnum, size);
 	if (size && fwrite(buf, size, 1, stdout) != 1)
 		die ("Could not write blob %s", sha1_to_hex(sha1));
 	printf("\n");
@@ -190,7 +190,7 @@ static void handle_commit(struct commit *commit, struct rev_info *rev)
 		reencoded = reencode_string(message, "UTF-8", encoding);
 	if (!commit->parents)
 		printf("reset %s\n", (const char*)commit->util);
-	printf("commit %s\nmark :%d\n%.*s\n%.*s\ndata %u\n%s",
+	printf("commit %s\nmark :%"PRIu32"\n%.*s\n%.*s\ndata %u\n%s",
 	       (const char *)commit->util, last_idnum,
 	       (int)(author_end - author), author,
 	       (int)(committer_end - committer), committer,
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index de1e8d1..e094e2d 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -519,7 +519,8 @@ static int get_pack(int xd[2], char **pack_lockfile)
 
 		if (read_pack_header(demux.out, &header))
 			die("protocol error: bad pack header");
-		snprintf(hdr_arg, sizeof(hdr_arg), "--pack_header=%u,%u",
+		snprintf(hdr_arg, sizeof(hdr_arg),
+			 "--pack_header=%"PRIu32",%"PRIu32,
 			 ntohl(header.hdr_version), ntohl(header.hdr_entries));
 		if (ntohl(header.hdr_entries) < unpack_limit)
 			do_keep = 0;
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 447d492..5e97355 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -590,7 +590,8 @@ static void write_pack_file(void)
 	free(written_list);
 	stop_progress(&progress_state);
 	if (written != nr_result)
-		die("wrote %u objects while expecting %u", written, nr_result);
+		die("wrote %"PRIu32" objects while expecting %"PRIu32,
+			written, nr_result);
 	/*
 	 * We have scanned through [0 ... i).  Since we have written
 	 * the correct number of objects,  the remaining [i ... nr_objects)
@@ -602,7 +603,8 @@ static void write_pack_file(void)
 		j += !e->idx.offset && !e->preferred_base;
 	}
 	if (j)
-		die("wrote %u objects as expected but %u unwritten", written, j);
+		die("wrote %"PRIu32" objects as expected but %"PRIu32
+			" unwritten", written, j);
 }
 
 static int locate_object_entry_hash(const unsigned char *sha1)
@@ -1718,7 +1720,8 @@ static int add_ref_tag(const char *path, const unsigned char *sha1, int flag, vo
 static void prepare_pack(int window, int depth)
 {
 	struct object_entry **delta_list;
-	uint32_t i, n, nr_deltas;
+	uint32_t i, nr_deltas;
+	unsigned n;
 
 	get_object_details();
 
@@ -1809,7 +1812,8 @@ static int git_pack_config(const char *k, const char *v, void *cb)
 	if (!strcmp(k, "pack.indexversion")) {
 		pack_idx_default_version = git_config_int(k, v);
 		if (pack_idx_default_version > 2)
-			die("bad pack.indexversion=%d", pack_idx_default_version);
+			die("bad pack.indexversion=%"PRIu32,
+				pack_idx_default_version);
 		return 0;
 	}
 	if (!strcmp(k, "pack.packsizelimit")) {
@@ -2243,7 +2247,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		prepare_pack(window, depth);
 	write_pack_file();
 	if (progress)
-		fprintf(stderr, "Total %u (delta %u), reused %u (delta %u)\n",
+		fprintf(stderr, "Total %"PRIu32" (delta %"PRIu32"),"
+			" reused %"PRIu32" (delta %"PRIu32")\n",
 			written, written_delta, reused, reused_delta);
 	return 0;
 }
diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c
index 85043d1..a891866 100644
--- a/builtin-unpack-objects.c
+++ b/builtin-unpack-objects.c
@@ -471,7 +471,8 @@ static void unpack_all(void)
 	if (ntohl(hdr->hdr_signature) != PACK_SIGNATURE)
 		die("bad pack file");
 	if (!pack_version_ok(hdr->hdr_version))
-		die("unknown pack file version %d", ntohl(hdr->hdr_version));
+		die("unknown pack file version %"PRIu32,
+			ntohl(hdr->hdr_version));
 	use(sizeof(struct pack_header));
 
 	if (!quiet)
diff --git a/index-pack.c b/index-pack.c
index 5ac91ba..25db5db 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -190,7 +190,8 @@ static void parse_pack_header(void)
 	if (hdr->hdr_signature != htonl(PACK_SIGNATURE))
 		die("pack signature mismatch");
 	if (!pack_version_ok(hdr->hdr_version))
-		die("pack version %d unsupported", ntohl(hdr->hdr_version));
+		die("pack version %"PRIu32" unsupported",
+			ntohl(hdr->hdr_version));
 
 	nr_objects = ntohl(hdr->hdr_entries);
 	use(sizeof(struct pack_header));
@@ -771,7 +772,8 @@ static int git_index_pack_config(const char *k, const char *v, void *cb)
 	if (!strcmp(k, "pack.indexversion")) {
 		pack_idx_default_version = git_config_int(k, v);
 		if (pack_idx_default_version > 2)
-			die("bad pack.indexversion=%d", pack_idx_default_version);
+			die("bad pack.indexversion=%"PRIu32,
+				pack_idx_default_version);
 		return 0;
 	}
 	return git_default_config(k, v, cb);
diff --git a/pack-check.c b/pack-check.c
index f489873..98a6d56 100644
--- a/pack-check.c
+++ b/pack-check.c
@@ -146,11 +146,11 @@ static void show_pack_info(struct packed_git *p)
 	for (i = 0; i <= MAX_CHAIN; i++) {
 		if (!chain_histogram[i])
 			continue;
-		printf("chain length = %d: %d object%s\n", i,
+		printf("chain length = %"PRIu32": %"PRIu32" object%s\n", i,
 		       chain_histogram[i], chain_histogram[i] > 1 ? "s" : "");
 	}
 	if (chain_histogram[0])
-		printf("chain length > %d: %d object%s\n", MAX_CHAIN,
+		printf("chain length > %d: %"PRIu32" object%s\n", MAX_CHAIN,
 		       chain_histogram[0], chain_histogram[0] > 1 ? "s" : "");
 }
 
diff --git a/receive-pack.c b/receive-pack.c
index b26f2e3..fa653b4 100644
--- a/receive-pack.c
+++ b/receive-pack.c
@@ -370,7 +370,8 @@ static const char *unpack(void)
 	hdr_err = parse_pack_header(&hdr);
 	if (hdr_err)
 		return hdr_err;
-	snprintf(hdr_arg, sizeof(hdr_arg), "--pack_header=%u,%u",
+	snprintf(hdr_arg, sizeof(hdr_arg),
+			"--pack_header=%"PRIu32",%"PRIu32,
 			ntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));
 
 	if (ntohl(hdr.hdr_entries) < unpack_limit) {
diff --git a/sha1_file.c b/sha1_file.c
index 191f814..7610c43 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -471,7 +471,7 @@ static int check_packed_git_idx(const char *path,  struct packed_git *p)
 		version = ntohl(hdr->idx_version);
 		if (version < 2 || version > 2) {
 			munmap(idx_map, idx_size);
-			return error("index file %s is version %d"
+			return error("index file %s is version %"PRIu32
 				     " and is not supported by this binary"
 				     " (try upgrading GIT to a newer version)",
 				     path, version);
@@ -682,14 +682,14 @@ static int open_packed_git_1(struct packed_git *p)
 	if (hdr.hdr_signature != htonl(PACK_SIGNATURE))
 		return error("file %s is not a GIT packfile", p->pack_name);
 	if (!pack_version_ok(hdr.hdr_version))
-		return error("packfile %s is version %u and not supported"
-			" (try upgrading GIT to a newer version)",
+		return error("packfile %s is version %"PRIu32" and not"
+			" supported (try upgrading GIT to a newer version)",
 			p->pack_name, ntohl(hdr.hdr_version));
 
 	/* Verify the pack matches its index. */
 	if (p->num_objects != ntohl(hdr.hdr_entries))
-		return error("packfile %s claims to have %u objects"
-			     " while index indicates %u objects",
+		return error("packfile %s claims to have %"PRIu32" objects"
+			     " while index indicates %"PRIu32" objects",
 			     p->pack_name, ntohl(hdr.hdr_entries),
 			     p->num_objects);
 	if (lseek(p->pack_fd, p->pack_size - sizeof(sha1), SEEK_SET) == -1)
@@ -1719,7 +1719,7 @@ off_t find_pack_entry_one(const unsigned char *sha1,
 	}
 
 	if (debug_lookup)
-		printf("%02x%02x%02x... lo %u hi %u nr %u\n",
+		printf("%02x%02x%02x... lo %u hi %u nr %"PRIu32"\n",
 		       sha1[0], sha1[1], sha1[2], lo, hi, p->num_objects);
 
 	if (use_lookup < 0)
diff --git a/show-index.c b/show-index.c
index 7253991..45bb535 100644
--- a/show-index.c
+++ b/show-index.c
@@ -68,7 +68,8 @@ int main(int argc, char **argv)
 						     ntohl(off64[1]);
 				off64_nr++;
 			}
-			printf("%" PRIuMAX " %s (%08x)\n", (uintmax_t) offset,
+			printf("%" PRIuMAX " %s (%08"PRIx32")\n",
+			       (uintmax_t) offset,
 			       sha1_to_hex(entries[i].sha1),
 			       ntohl(entries[i].crc));
 		}
-- 
1.5.6
Junio C Hamano· Jul 4, 2008, 08:59 UTC · re: Ramsay Jones · lore

Re: [PATCH/RFC] Fix some warnings (on cygwin) to allow -Werror

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> Despite the subject line, this should be useful on any system for
> which uint32_t is defined to be unsigned long rather than
> unsigned int. (and where the return type of htonl() is similarly
> defined).
Correct.  Perhaps "Use %PRIu32 and such to print integers of fixed length"?

The only worry I have with this patch is with systems with not-so-ANSI headers; as you noticed, we already have:

        #ifndef PRIuMAX
        #define PRIuMAX "llu"
        #endif

in git-compat-util.h, which does include <inttypes.h>, so PRIu32 and friends could be problematic on these platforms.

Show 7 quoted lines
> @@ -1718,7 +1720,8 @@ static int add_ref_tag(const char *path, const unsigned char *sha1, int flag, vo
>  static void prepare_pack(int window, int depth)
>  {
>  	struct object_entry **delta_list;
> -	uint32_t i, n, nr_deltas;
> +	uint32_t i, nr_deltas;
> +	unsigned n;
Hmm.  Is this change necessary?
Ramsay Jones· Jul 5, 2008, 20:35 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] Fix some warnings (on cygwin) to allow -Werror

Junio C Hamano wrote:
Show 19 quoted lines
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> 
>> Despite the subject line, this should be useful on any system for
>> which uint32_t is defined to be unsigned long rather than
>> unsigned int. (and where the return type of htonl() is similarly
>> defined).
> 
> Correct.  Perhaps "Use %PRIu32 and such to print integers of fixed length"?
> 
> The only worry I have with this patch is with systems with not-so-ANSI
> headers; as you noticed, we already have:
> 
>         #ifndef PRIuMAX
>         #define PRIuMAX "llu"
>         #endif
> 
> in git-compat-util.h, which does include <inttypes.h>, so PRIu32 and
> friends could be problematic on these platforms.
> 

Yes, I had the same worry, since I did not know which system(s) required the above #ifdef and, perhaps more important, why. Having said that, I suspect (hope?) that the affected system(s) are somewhat rare. I guess it would not be long before someone complained otherwise...

[Note: I can't just blame the above #ifdef because I don't have a real git repo ;-) - it would simply blame me!]

Show 10 quoted lines
>> @@ -1718,7 +1720,8 @@ static int add_ref_tag(const char *path, const unsigned char *sha1, int flag, vo
>>  static void prepare_pack(int window, int depth)
>>  {
>>  	struct object_entry **delta_list;
>> -	uint32_t i, n, nr_deltas;
>> +	uint32_t i, nr_deltas;
>> +	unsigned n;
> 
> Hmm.  Is this change necessary?
> 
Yes, otherwise:

builtin-pack-objects.c: In function `prepare_pack': builtin-pack-objects.c:1760: warning: passing arg 2 of `find_deltas' from incompatible pointer type make: *** [builtin-pack-objects.o] Error 1

Note that ll_find_deltas() is #defined to find_deltas() in the #else arm of #ifdef THREADED_DELTA_SEARCH, and find_deltas() takes an "unsigned *"

ATB,
Ramsay Jones
Junio C Hamano· Jul 6, 2008, 00:57 UTC · re: Ramsay Jones · lore

Re: [PATCH/RFC] Fix some warnings (on cygwin) to allow -Werror

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
Show 20 quoted lines
> Junio C Hamano wrote:
> ...
>>> @@ -1718,7 +1720,8 @@ static int add_ref_tag(const char *path, const unsigned char *sha1, int flag, vo
>>>  static void prepare_pack(int window, int depth)
>>>  {
>>>  	struct object_entry **delta_list;
>>> -	uint32_t i, n, nr_deltas;
>>> +	uint32_t i, nr_deltas;
>>> +	unsigned n;
>> 
>> Hmm.  Is this change necessary?
>
> Yes, otherwise:
>
> builtin-pack-objects.c: In function `prepare_pack':
> builtin-pack-objects.c:1760: warning: passing arg 2 of `find_deltas' from incompatible pointer type
> make: *** [builtin-pack-objects.o] Error 1
>
> Note that ll_find_deltas() is #defined to find_deltas() in the #else arm
> of #ifdef THREADED_DELTA_SEARCH, and find_deltas() takes an "unsigned *"

Ah, I missed that. So it is not just warning squelch but is a bugfix in case unsigned and uint32_t are of different sizes.

Ramsay Jones· Jul 8, 2008, 22:21 UTC · re: Junio C Hamano · lore

Re: [PATCH/RFC] Fix some warnings (on cygwin) to allow -Werror

Junio C Hamano wrote:
Show 24 quoted lines
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> 
>> Junio C Hamano wrote:
>> ...
>>>> @@ -1718,7 +1720,8 @@ static int add_ref_tag(const char *path, const unsigned char *sha1, int flag, vo
>>>>  static void prepare_pack(int window, int depth)
>>>>  {
>>>>  	struct object_entry **delta_list;
>>>> -	uint32_t i, n, nr_deltas;
>>>> +	uint32_t i, nr_deltas;
>>>> +	unsigned n;
>>> Hmm.  Is this change necessary?
>> Yes, otherwise:
>>
>> builtin-pack-objects.c: In function `prepare_pack':
>> builtin-pack-objects.c:1760: warning: passing arg 2 of `find_deltas' from incompatible pointer type
>> make: *** [builtin-pack-objects.o] Error 1
>>
>> Note that ll_find_deltas() is #defined to find_deltas() in the #else arm
>> of #ifdef THREADED_DELTA_SEARCH, and find_deltas() takes an "unsigned *"
> 
> Ah, I missed that.  So it is not just warning squelch but is a bugfix in
> case unsigned and uint32_t are of different sizes.
> 
Yes, but I didn't think about that when I "fixed" it ;-)
ATB,
Ramsay Jones
Steffen Prohaska· Jul 4, 2008, 09:46 UTC · re: Ramsay Jones · lore

Re: [PATCH/RFC] Fix some warnings (on cygwin) to allow -Werror

On Jul 3, 2008, at 5:52 PM, Ramsay Jones wrote:
Show 8 quoted lines
>
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
>
> Despite the subject line, this should be useful on any system for
> which uint32_t is defined to be unsigned long rather than
> unsigned int. (and where the return type of htonl() is similarly
> defined).
For MINGW, we fixed these warnings in the compat layer, see cd800eecc2:
Show changes to compat/mingw.h +4 −0
diff --git a/compat/mingw.h b/compat/mingw.h
index a87cc96..6bc049a 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -186,6 +186,10 @@ pid_t mingw_spawnvpe(const char *cmd, const char  
**argv, char **env);
  void mingw_execvp(const char *cmd, char *const *argv);
  #define execvp mingw_execvp

+static inline unsigned int git_ntohl(unsigned int x)
+{ return (unsigned int)ntohl(x); }
+#define ntohl git_ntohl
+
  sig_handler_t mingw_signal(int sig, sig_handler_t handler);
  #define signal mingw_signal

	Steffen
Ramsay Jones· Jul 5, 2008, 20:56 UTC · re: Steffen Prohaska · lore

Re: [PATCH/RFC] Fix some warnings (on cygwin) to allow -Werror

Steffen Prohaska wrote:
Show 28 quoted lines
> On Jul 3, 2008, at 5:52 PM, Ramsay Jones wrote:
> 
>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>> ---
>>
>> Despite the subject line, this should be useful on any system for
>> which uint32_t is defined to be unsigned long rather than
>> unsigned int. (and where the return type of htonl() is similarly
>> defined).
> 
> For MINGW, we fixed these warnings in the compat layer, see cd800eecc2:
> 
> diff --git a/compat/mingw.h b/compat/mingw.h
> index a87cc96..6bc049a 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -186,6 +186,10 @@ pid_t mingw_spawnvpe(const char *cmd, const char  
> **argv, char **env);
>   void mingw_execvp(const char *cmd, char *const *argv);
>   #define execvp mingw_execvp
> 
> +static inline unsigned int git_ntohl(unsigned int x)
> +{ return (unsigned int)ntohl(x); }
> +#define ntohl git_ntohl
> +
>   sig_handler_t mingw_signal(int sig, sig_handler_t handler);
>   #define signal mingw_signal
> 

Yes, I had something similar at one point (but not in compat). However, not all of these warnings are solved by the above. For example, the changes to builtin-fast-export.c, builtin-pack-objects.c, and pack-check.c do not involve ntohl() at all. (some hunks in other files do not involve ntohl() either).

When the PRIu32 macro idea was suggested (I think it was Simon 'corecode' Schubert who first mentioned it), I thought it would look so ugly, no one would agree to such a patch... Having implemented it, well I guess it's not quite as bad as I feared ;-) YMMV!

ATB,
Ramsay Jones

← back to recent threads