[PATCH] bulk-checkin: fix sign compare warnings

[PATCH] bulk-checkin: fix sign compare warnings

[Tuomas Ahola]

In file bulk-checkin.c, three warnings are emitted by
"-Wsign-compare", two of which are caused by trivial loop iterator
type mismatches.  The third one is also an uncomplicated case for
which a simple cast is a sufficient remedy.

Fix issues accordingly, and enable sign compare warnings for the file.

Signed-off-by: Tuomas Ahola <taahol@utu.fi>
---
 bulk-checkin.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 20f2da67b9..0133427132 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -3,7 +3,6 @@
  */
 
 #define USE_THE_REPOSITORY_VARIABLE
-#define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "git-compat-util.h"
 #include "bulk-checkin.h"
@@ -56,7 +55,6 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)
 {
 	unsigned char hash[GIT_MAX_RAWSZ];
 	struct strbuf packname = STRBUF_INIT;
-	int i;
 
 	if (!state->f)
 		return;
@@ -82,7 +80,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)
 	finish_tmp_packfile(&packname, state->pack_tmp_name,
 			    state->written, state->nr_written,
 			    &state->pack_idx_opts, hash);
-	for (i = 0; i < state->nr_written; i++)
+	for (uint32_t i = 0; i < state->nr_written; i++)
 		free(state->written[i]);
 
 clear_exit:
@@ -131,14 +129,12 @@ static void flush_batch_fsync(void)
 
 static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)
 {
-	int i;
-
 	/* The object may already exist in the repository */
 	if (repo_has_object_file(the_repository, oid))
 		return 1;
 
 	/* Might want to keep the list sorted */
-	for (i = 0; i < state->nr_written; i++)
+	for (uint32_t i = 0; i < state->nr_written; i++)
 		if (oideq(&state->written[i]->oid, oid))
 			return 1;
 
@@ -192,7 +188,7 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
 			offset += rsize;
 			if (*already_hashed_to < offset) {
 				size_t hsize = offset - *already_hashed_to;
-				if (rsize < hsize)
+				if ((size_t)rsize < hsize)
 					hsize = rsize;
 				if (hsize)
 					git_hash_update(ctx, ibuf, hsize);

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
-- 
2.30.2

Re: [PATCH] bulk-checkin: fix sign compare warnings

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 2578 bytes --]

Tuomas Ahola <taahol@utu.fi> writes:

> In file bulk-checkin.c, three warnings are emitted by
> "-Wsign-compare", two of which are caused by trivial loop iterator
> type mismatches.  The third one is also an uncomplicated case for
> which a simple cast is a sufficient remedy.
>

Nit: it would be nice if you expanded on why 'a simple cast is a
sufficient remedy' and more importantly how that casting is safe.

> Fix issues accordingly, and enable sign compare warnings for the file.
>
> Signed-off-by: Tuomas Ahola <taahol@utu.fi>
> ---
>  bulk-checkin.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 20f2da67b9..0133427132 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -3,7 +3,6 @@
>   */
>
>  #define USE_THE_REPOSITORY_VARIABLE
> -#define DISABLE_SIGN_COMPARE_WARNINGS
>
>  #include "git-compat-util.h"
>  #include "bulk-checkin.h"
> @@ -56,7 +55,6 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)
>  {
>  	unsigned char hash[GIT_MAX_RAWSZ];
>  	struct strbuf packname = STRBUF_INIT;
> -	int i;
>
>  	if (!state->f)
>  		return;
> @@ -82,7 +80,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)
>  	finish_tmp_packfile(&packname, state->pack_tmp_name,
>  			    state->written, state->nr_written,
>  			    &state->pack_idx_opts, hash);
> -	for (i = 0; i < state->nr_written; i++)
> +	for (uint32_t i = 0; i < state->nr_written; i++)
>  		free(state->written[i]);
>
>  clear_exit:
> @@ -131,14 +129,12 @@ static void flush_batch_fsync(void)
>
>  static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)
>  {
> -	int i;
> -
>  	/* The object may already exist in the repository */
>  	if (repo_has_object_file(the_repository, oid))
>  		return 1;
>
>  	/* Might want to keep the list sorted */
> -	for (i = 0; i < state->nr_written; i++)
> +	for (uint32_t i = 0; i < state->nr_written; i++)
>  		if (oideq(&state->written[i]->oid, oid))
>  			return 1;
>
> @@ -192,7 +188,7 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
>  			offset += rsize;
>  			if (*already_hashed_to < offset) {
>  				size_t hsize = offset - *already_hashed_to;
> -				if (rsize < hsize)
> +				if ((size_t)rsize < hsize)

Something I found peculiar here is that `rsize` is of type ssize_t'.
But it only seems to store a positive value.

>  					hsize = rsize;
>  				if (hsize)
>  					git_hash_update(ctx, ibuf, hsize);
>
> base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
> --
> 2.30.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

[PATCH v2] bulk-checkin: fix sign compare warnings

[Tuomas Ahola]

In file bulk-checkin.c, three warnings are emitted by
"-Wsign-compare", two of which are caused by trivial loop iterator
type mismatches.  The third one is also an uncomplicated case for
which a simple cast is a safe and sufficient action as the variable in
question only holds positive values (from sizeof() expression).

Fix issues accordingly, and enable sign compare warnings for the file.

Signed-off-by: Tuomas Ahola <taahol@utu.fi>
---
Intervall-diff mot v1:
1:  25b56dae76 ! 1:  289f3a0278 bulk-checkin: fix sign compare warnings
    @@ Commit message
         In file bulk-checkin.c, three warnings are emitted by
         "-Wsign-compare", two of which are caused by trivial loop iterator
         type mismatches.  The third one is also an uncomplicated case for
    -    which a simple cast is a sufficient remedy.
    +    which a simple cast is a safe and sufficient action as the variable in
    +    question only holds positive values (from sizeof() expression).
     
         Fix issues accordingly, and enable sign compare warnings for the file.
     

 bulk-checkin.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 20f2da67b9..0133427132 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -3,7 +3,6 @@
  */
 
 #define USE_THE_REPOSITORY_VARIABLE
-#define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "git-compat-util.h"
 #include "bulk-checkin.h"
@@ -56,7 +55,6 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)
 {
 	unsigned char hash[GIT_MAX_RAWSZ];
 	struct strbuf packname = STRBUF_INIT;
-	int i;
 
 	if (!state->f)
 		return;
@@ -82,7 +80,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)
 	finish_tmp_packfile(&packname, state->pack_tmp_name,
 			    state->written, state->nr_written,
 			    &state->pack_idx_opts, hash);
-	for (i = 0; i < state->nr_written; i++)
+	for (uint32_t i = 0; i < state->nr_written; i++)
 		free(state->written[i]);
 
 clear_exit:
@@ -131,14 +129,12 @@ static void flush_batch_fsync(void)
 
 static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)
 {
-	int i;
-
 	/* The object may already exist in the repository */
 	if (repo_has_object_file(the_repository, oid))
 		return 1;
 
 	/* Might want to keep the list sorted */
-	for (i = 0; i < state->nr_written; i++)
+	for (uint32_t i = 0; i < state->nr_written; i++)
 		if (oideq(&state->written[i]->oid, oid))
 			return 1;
 
@@ -192,7 +188,7 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
 			offset += rsize;
 			if (*already_hashed_to < offset) {
 				size_t hsize = offset - *already_hashed_to;
-				if (rsize < hsize)
+				if ((size_t)rsize < hsize)
 					hsize = rsize;
 				if (hsize)
 					git_hash_update(ctx, ibuf, hsize);

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
-- 
2.30.2

Re: [PATCH v2] bulk-checkin: fix sign compare warnings

[Junio C Hamano]

Tuomas Ahola <taahol@utu.fi> writes:

> In file bulk-checkin.c, three warnings are emitted by
> "-Wsign-compare", two of which are caused by trivial loop iterator
> type mismatches.  The third one is also an uncomplicated case for
> which a simple cast is a safe and sufficient action as the variable in
> question only holds positive values (from sizeof() expression).

The point of the sign-compare is that a positive value that is
assigned to a signed variable may wrap around to become negative,
causing a comparison with an unsigned type with the same size to
fail.

So "only holds positive" is not a good enough explanation for the
reason why this workaround for the "-Wsign-compare" false-positive [*]
does not make things too bad.  The key thing is that the value
assigned to this "ssize_t rsize" variable is a small non-negative
value that can fit both size_t and ssize_t.


[Footnote]

 * If we take -Wsign-compare too literally, it is warning every time
   a signed quantity and an unsigned quantity is being compared, so
   we could argue that there is no false-positive.  But that is an
   obviously pretty useless warning, when we can trivially tell that
   the value in a signed variable cannot have wrapped around.

Re: [PATCH] bulk-checkin: fix sign compare warnings

[Jeff King]

On Fri, Mar 21, 2025 at 05:08:06PM -0400, Karthik Nayak wrote:

> > @@ -192,7 +188,7 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
> >  			offset += rsize;
> >  			if (*already_hashed_to < offset) {
> >  				size_t hsize = offset - *already_hashed_to;
> > -				if (rsize < hsize)
> > +				if ((size_t)rsize < hsize)
> 
> Something I found peculiar here is that `rsize` is of type ssize_t'.
> But it only seems to store a positive value.

I assumed it was ssize_t because it would hold the result of a read
call. But it doesn't! We put that into the "read_result" variable.

So it could just be a size_t in the first place. And indeed it is better
as one, because we assign from "size", which is itself a size_t. We do
not yet warn about type mismatches outside of comparisons, but really it
is equally bad.

However, if you switch it, then we get a different -Wsign-compare
problem: we compare "rsize" and "read_result". So you still have to
cast, but at a different spot.

If we are doing this a lot (and really this conversion is necessary any
time you look at the outcome of a read call), I do still wonder if we
should have a helper like:

static inline int safe_scast(ssize_t ret, size_t *out)
{
	if (ret < 0)
		return 0;
	/* cast is safe because of check above */
	*out = (size_t)ret;
	return 1;
}

(yes, I know the name is lousy). That would allow something like this:

diff --git a/bulk-checkin.c b/bulk-checkin.c
index f6f79cb9e2..fbffc7c8d6 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -178,9 +178,10 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
 
 	while (status != Z_STREAM_END) {
 		if (size && !s.avail_in) {
-			ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
-			ssize_t read_result = read_in_full(fd, ibuf, rsize);
-			if (read_result < 0)
+			size_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
+			size_t read_result;
+
+			if (!safe_scast(read_in_full(fd, ibuf, rsize), &read_result))
 				die_errno("failed to read from '%s'", path);
 			if (read_result != rsize)
 				die("failed to read %d bytes from '%s'",

Though it does kind of obscure the call to read_in_full(). You can use
two variables, like:

  ssize_t read_result;
  size_t bytes_read;

  read_result = read_in_full(fd, ibuf, rsize);
  if (!safe_scast(read_result, &bytes_read))
	die_errno(...);

which is a bit more verbose but perhaps clearer.

This reminded me a bit of the issues we had with write_in_full() before,
where:

  if (write_in_full(fd, buf, len) < len)

behaves unexpectedly because of integer conversions. There the solution
was to never check against "len", because write_in_full() either writes
everything or returns an error. So:

  if (write_in_full(fd, buf, len) < 0)

is correct and sufficient.

But alas, we can't do the same here, because reading returns three
cases: error, a full read, or a partial read (maybe even EOF!). So we
really do need to record and compare the return value between what we
asked for and what we got.

-Peff

Re: [PATCH] bulk-checkin: fix sign compare warnings

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 4054 bytes --]

Jeff King <peff@peff.net> writes:

> On Fri, Mar 21, 2025 at 05:08:06PM -0400, Karthik Nayak wrote:
>
>> > @@ -192,7 +188,7 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
>> >  			offset += rsize;
>> >  			if (*already_hashed_to < offset) {
>> >  				size_t hsize = offset - *already_hashed_to;
>> > -				if (rsize < hsize)
>> > +				if ((size_t)rsize < hsize)
>>
>> Something I found peculiar here is that `rsize` is of type ssize_t'.
>> But it only seems to store a positive value.
>
> I assumed it was ssize_t because it would hold the result of a read
> call. But it doesn't! We put that into the "read_result" variable.
>
> So it could just be a size_t in the first place. And indeed it is better
> as one, because we assign from "size", which is itself a size_t. We do
> not yet warn about type mismatches outside of comparisons, but really it
> is equally bad.

Nice, thanks for exploring this thought out more. I did look at the
code, but was more cursory.

> However, if you switch it, then we get a different -Wsign-compare
> problem: we compare "rsize" and "read_result". So you still have to
> cast, but at a different spot.
>

True. But this would be better in my regards, since this would directly
follow the

  if (read_result < 0)
     die_errno("failed to read from '%s'", path);

code, so a `if ((size_t)read_result != rsize)` here makes logical sense
since we can clearly see that this section is only reached when
`read_result` has a positive value.

> If we are doing this a lot (and really this conversion is necessary any
> time you look at the outcome of a read call), I do still wonder if we
> should have a helper like:
>
> static inline int safe_scast(ssize_t ret, size_t *out)
> {
> 	if (ret < 0)
> 		return 0;
> 	/* cast is safe because of check above */
> 	*out = (size_t)ret;
> 	return 1;
> }
>
> (yes, I know the name is lousy). That would allow something like this:
>
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index f6f79cb9e2..fbffc7c8d6 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -178,9 +178,10 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
>
>  	while (status != Z_STREAM_END) {
>  		if (size && !s.avail_in) {
> -			ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
> -			ssize_t read_result = read_in_full(fd, ibuf, rsize);
> -			if (read_result < 0)
> +			size_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
> +			size_t read_result;
> +
> +			if (!safe_scast(read_in_full(fd, ibuf, rsize), &read_result))
>  				die_errno("failed to read from '%s'", path);
>  			if (read_result != rsize)
>  				die("failed to read %d bytes from '%s'",
>

This does look nice, but I'm worried something like `safe_scast` would
just not be used througout the codebase, causing inconsistencies. But I
think we can drive that through reviews.

> Though it does kind of obscure the call to read_in_full(). You can use
> two variables, like:
>
>   ssize_t read_result;
>   size_t bytes_read;
>
>   read_result = read_in_full(fd, ibuf, rsize);
>   if (!safe_scast(read_result, &bytes_read))
> 	die_errno(...);
>
> which is a bit more verbose but perhaps clearer.

Yeah this is much better too.

> This reminded me a bit of the issues we had with write_in_full() before,
> where:
>
>   if (write_in_full(fd, buf, len) < len)
>
> behaves unexpectedly because of integer conversions. There the solution
> was to never check against "len", because write_in_full() either writes
> everything or returns an error. So:
>
>   if (write_in_full(fd, buf, len) < 0)
>
> is correct and sufficient.
>
> But alas, we can't do the same here, because reading returns three
> cases: error, a full read, or a partial read (maybe even EOF!). So we
> really do need to record and compare the return value between what we
> asked for and what we got.
>

This is to some extent a flaw in the way errors are generally
structured where the error indication (-1 here) and a potential result
(bytes read) are combined into a single return.

It is unfortunate indeed.

> -Peff

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH] bulk-checkin: fix sign compare warnings

[Jeff King]

On Mon, Mar 24, 2025 at 07:48:59PM +0000, Karthik Nayak wrote:

> > However, if you switch it, then we get a different -Wsign-compare
> > problem: we compare "rsize" and "read_result". So you still have to
> > cast, but at a different spot.
> 
> True. But this would be better in my regards, since this would directly
> follow the
> 
>   if (read_result < 0)
>      die_errno("failed to read from '%s'", path);
> 
> code, so a `if ((size_t)read_result != rsize)` here makes logical sense
> since we can clearly see that this section is only reached when
> `read_result` has a positive value.

Yeah, agreed. And after doing that, it's probably the right stopping
point for this patch.

I do think the safe_scast() thing could be a good general tool for this
kind of case, but I'd rather not hold up an immediate fix for it.

-Peff

[PATCH v3] bulk-checkin: fix sign compare warnings

[Tuomas Ahola]

In file bulk-checkin.c, three warnings are emitted by
"-Wsign-compare", two of which are caused by trivial loop iterator
type mismatches.  For the third case, the type of `rsize` from

			ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);

can be changed to size_t as both options of the ternary expression are
unsigned and the signedness of the variable isn't really needed
anywhere.

To prevent `read_result != rsize` making a clash, it is to be noted
that `read_result` is checked not to hold negative values.  Therefore
casting the variable to size_t is a safe operation and enough to
remove the sign-compare warning.

Fix issues accordingly, and remove `DISABLE_SIGN_COMPARE_WARNINGS` to
enable "-Wsign-compare" for the file.

Signed-off-by: Tuomas Ahola <taahol@utu.fi>
---

Notes:
    Okay, I think I got it know.  Thanks for bearing with me.
    
    Range-diff against v2:
    
          ## bulk-checkin.c ##
         @@
           */
        @@ bulk-checkin.c: static void flush_batch_fsync(void)
          			return 1;
    
         @@ bulk-checkin.c: static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
        +
        + 	while (status != Z_STREAM_END) {
        + 		if (size && !s.avail_in) {
        +-			ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
        ++			size_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
        + 			ssize_t read_result = read_in_full(fd, ibuf, rsize);
        + 			if (read_result < 0)
        + 				die_errno("failed to read from '%s'", path);
        +-			if (read_result != rsize)
        +-				die("failed to read %d bytes from '%s'",
        +-				    (int)rsize, path);
        ++			if ((size_t)read_result != rsize)
        ++				die("failed to read %u bytes from '%s'",
        ++				    (unsigned)rsize, path);
          			offset += rsize;
          			if (*already_hashed_to < offset) {
          				size_t hsize = offset - *already_hashed_to;
        --				if (rsize < hsize)
        -+				if ((size_t)rsize < hsize)
        - 					hsize = rsize;
        - 				if (hsize)
        - 					git_hash_update(ctx, ibuf, hsize);

 bulk-checkin.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 20f2da67b9..a5a3395188 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -3,7 +3,6 @@
  */
 
 #define USE_THE_REPOSITORY_VARIABLE
-#define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "git-compat-util.h"
 #include "bulk-checkin.h"
@@ -56,7 +55,6 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)
 {
 	unsigned char hash[GIT_MAX_RAWSZ];
 	struct strbuf packname = STRBUF_INIT;
-	int i;
 
 	if (!state->f)
 		return;
@@ -82,7 +80,7 @@ static void flush_bulk_checkin_packfile(struct bulk_checkin_packfile *state)
 	finish_tmp_packfile(&packname, state->pack_tmp_name,
 			    state->written, state->nr_written,
 			    &state->pack_idx_opts, hash);
-	for (i = 0; i < state->nr_written; i++)
+	for (uint32_t i = 0; i < state->nr_written; i++)
 		free(state->written[i]);
 
 clear_exit:
@@ -131,14 +129,12 @@ static void flush_batch_fsync(void)
 
 static int already_written(struct bulk_checkin_packfile *state, struct object_id *oid)
 {
-	int i;
-
 	/* The object may already exist in the repository */
 	if (repo_has_object_file(the_repository, oid))
 		return 1;
 
 	/* Might want to keep the list sorted */
-	for (i = 0; i < state->nr_written; i++)
+	for (uint32_t i = 0; i < state->nr_written; i++)
 		if (oideq(&state->written[i]->oid, oid))
 			return 1;
 
@@ -182,13 +178,13 @@ static int stream_blob_to_pack(struct bulk_checkin_packfile *state,
 
 	while (status != Z_STREAM_END) {
 		if (size && !s.avail_in) {
-			ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
+			size_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
 			ssize_t read_result = read_in_full(fd, ibuf, rsize);
 			if (read_result < 0)
 				die_errno("failed to read from '%s'", path);
-			if (read_result != rsize)
-				die("failed to read %d bytes from '%s'",
-				    (int)rsize, path);
+			if ((size_t)read_result != rsize)
+				die("failed to read %u bytes from '%s'",
+				    (unsigned)rsize, path);
 			offset += rsize;
 			if (*already_hashed_to < offset) {
 				size_t hsize = offset - *already_hashed_to;

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
-- 
2.30.2

Re: [PATCH v3] bulk-checkin: fix sign compare warnings

[Jeff King]

On Mon, Mar 24, 2025 at 11:47:03PM +0200, Tuomas Ahola wrote:

> In file bulk-checkin.c, three warnings are emitted by
> "-Wsign-compare", two of which are caused by trivial loop iterator
> type mismatches.  For the third case, the type of `rsize` from
> 
> 			ssize_t rsize = size < sizeof(ibuf) ? size : sizeof(ibuf);
> 
> can be changed to size_t as both options of the ternary expression are
> unsigned and the signedness of the variable isn't really needed
> anywhere.
> 
> To prevent `read_result != rsize` making a clash, it is to be noted
> that `read_result` is checked not to hold negative values.  Therefore
> casting the variable to size_t is a safe operation and enough to
> remove the sign-compare warning.

Thanks, this description (and the matching changes in the patch) look
good to me.

-Peff