You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@xbc.nu> on 2004/06/09 01:56:01 UTC
Warnings in fs_fs compilation on Win32
C:\Home\brane\src\svn\repo\subversion\libsvn_fs_fs\fs_fs.c(501) : warning C4244: '=' : conversion from '__int64 ' to 'unsigned int ', possible loss of data
C:\Home\brane\src\svn\repo\subversion\libsvn_fs_fs\fs_fs.c(508) : warning C4244: '=' : conversion from '__int64 ' to 'unsigned int ', possible loss of data
C:\Home\brane\src\svn\repo\subversion\libsvn_fs_fs\fs_fs.c(853) : warning C4244: '=' : conversion from '__int64 ' to 'unsigned int ', possible loss of data
C:\Home\brane\src\svn\repo\subversion\libsvn_fs_fs\fs_fs.c(1284) : warning C4244: '=' : conversion from '__int64 ' to 'unsigned int ', possible loss of data
C:\Home\brane\src\svn\repo\subversion\libsvn_fs_fs\fs_fs.c(2814) : warning C4244: '=' : conversion from '__int64 ' to 'unsigned int ', possible loss of data
C:\Home\brane\src\svn\repo\subversion\libsvn_fs_fs\fs_fs.c(2817) : warning C4244: '=' : conversion from '__int64 ' to 'unsigned int ', possible loss of data
I think the representation size should be an svn_filesize_t, not an
unsigned int, especially since it's generated using apr_atoi64 in many
places.
C:\Home\brane\src\svn\repo\subversion\libsvn_fs_fs\fs_fs.c(3604) : warning C4018: '<=' : signed/unsigned mismatch
strlen returns a size_t, not an apr_ssize_t.
Hmm, and I see you're computing 'strlen(PATH_EXT_TXN)' three times in a
loop. Premature optimisation may be the root of all evil, but we should
at least /look/ like we're trying... :-p
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Warnings in fs_fs compilation on Win32
Posted by Greg Hudson <gh...@MIT.EDU>.
On Thu, 2004-06-10 at 02:40, Branko Čibej wrote:
> I /think/ all three casts are safe.
Well, the FS files aren't untrusted data.
The patch looks fine.
I was thinking of proposing that we add -Wsign-compare to the gcc
warning flags in configure.in, but it doesn't seem to be present in gcc
2.95. And even gcc 3.4 doesn't seem to have an option to warn about
implicit narrowing of integer types.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Warnings in fs_fs compilation on Win32
Posted by Branko Čibej <br...@xbc.nu>.
Branko Čibej wrote:
>Greg Hudson wrote:
>
>
>>But you're right; the size and expanded_size fields of a
>>representation_t should be svn_filesize_t's, because of what they
>>represent. I will fix, but I can't find a gcc 3.2.3 option to warn
>>about implicit type narrowing, so I can't guarantee that I've fixed all
>>the warnings.
>>
>>
>That's fine. MSVC will warn about it, so I can fix the remaining ones
>once the types are changed. I didn't want to meddle with the FSFS
>structures myself.
>
>
Here's a patch that removes the last warnings from fs_fs.c:
Index: subversion/libsvn_fs_fs/fs_fs.c
===================================================================
--- subversion/libsvn_fs_fs/fs_fs.c (revision 9944)
+++ subversion/libsvn_fs_fs/fs_fs.c (working copy)
@@ -846,11 +846,11 @@
str = apr_strtok (NULL, " ", &last_str);
if (! str) goto err;
- rep_args->base_offset = apr_atoi64 (str);
+ rep_args->base_offset = (apr_off_t) apr_atoi64 (str);
str = apr_strtok (NULL, " ", &last_str);
if (! str) goto err;
- rep_args->base_length = apr_atoi64 (str);
+ rep_args->base_length = (apr_size_t) apr_atoi64 (str);
*rep_args_p = rep_args;
return SVN_NO_ERROR;
@@ -1281,7 +1281,7 @@
copy_len = remaining;
rs = rb->src_state;
if (((apr_off_t) copy_len) > rs->end - rs->off)
- copy_len = rs->end - rs->off;
+ copy_len = (apr_size_t) (rs->end - rs->off);
SVN_ERR (svn_io_file_read_full (rs->file, cur, copy_len, NULL,
rb->pool));
rs->off += copy_len;
I /think/ all three casts are safe. The last one obviously is, because
the number gets checked safely just before the assignment. The first one
doesn't actually trigger a warning on Win32, where apr_off_t has 64
bits, but could do on platforms without largefile support. The second
one is a case of reading what you wrote before, so it should be safe, too..
Of course, ideally we'd always check the value against the bounds of the
narrower type before a narrowing cast, but hey, this isn't flight
control software (yet :-).
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Warnings in fs_fs compilation on Win32
Posted by Branko Čibej <br...@xbc.nu>.
Greg Hudson wrote:
>On Tue, 2004-06-08 at 21:56, Branko ÄŒibej wrote:
>
>
>>I think the representation size should be an svn_filesize_t, not an
>>unsigned int, especially since it's generated using apr_atoi64 in many
>>places.
>>
>>
>
>It's not an unsigned int, it's an apr_size_t, which varies from platform
>to platform. It's not like there's an ascii to apr_size_t conversion
>function.
>
>
Right, I didn't look at the struct definition, just at the error message.
>But you're right; the size and expanded_size fields of a
>representation_t should be svn_filesize_t's, because of what they
>represent. I will fix, but I can't find a gcc 3.2.3 option to warn
>about implicit type narrowing, so I can't guarantee that I've fixed all
>the warnings.
>
>
That's fine. MSVC will warn about it, so I can fix the remaining ones
once the types are changed. I didn't want to meddle with the FSFS
structures myself.
>>C:\Home\brane\src\svn\repo\subversion\libsvn_fs_fs\fs_fs.c(3604) : warning C4018: '<=' : signed/unsigned mismatch
>>
>>
>>strlen returns a size_t, not an apr_ssize_t.
>>
>>
>
>C is such a bore some times.
>
It is rather, isn't it.
> (int) -1 < (unsigned int) 3 evaluates to
>0. In this case, the signed variable is always positive, of course, but
>I can see why the warning exists.
>
>
Obviously a non-brain-dead compiler should be able to figure that out...
>>Hmm, and I see you're computing 'strlen(PATH_EXT_TXN)' three times in a
>>loop. Premature optimisation may be the root of all evil, but we should
>>at least /look/ like we're trying... :-p
>>
>>
>I can't imagine this would ever cause a perceptible speed difference,
>and therefore I would say it's wrong to compromise the code, even a
>little bit, in the name of saving cycles. But in this case the code
>also looks a little messy as a result of all those strlen() calls, so I
>will change that.
>
>
Yeah, the "messiness" is what caught my eye, too. Heh, GCC 3.3 will
optimize away a strlen on a static string, I believe, replacing it with
a constant. :-)
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: Warnings in fs_fs compilation on Win32
Posted by Greg Hudson <gh...@MIT.EDU>.
On Tue, 2004-06-08 at 21:56, Branko Čibej wrote:
> C:\Home\brane\src\svn\repo\subversion\libsvn_fs_fs\fs_fs.c(501) : warning C4244: '=' : conversion from '__int64 ' to 'unsigned int ', possible loss of data
> C:\Home\brane\src\svn\repo\subversion\libsvn_fs_fs\fs_fs.c(508) : warning C4244: '=' : conversion from '__int64 ' to 'unsigned int ', possible loss of data
> C:\Home\brane\src\svn\repo\subversion\libsvn_fs_fs\fs_fs.c(853) : warning C4244: '=' : conversion from '__int64 ' to 'unsigned int ', possible loss of data
> C:\Home\brane\src\svn\repo\subversion\libsvn_fs_fs\fs_fs.c(1284) : warning C4244: '=' : conversion from '__int64 ' to 'unsigned int ', possible loss of data
> C:\Home\brane\src\svn\repo\subversion\libsvn_fs_fs\fs_fs.c(2814) : warning C4244: '=' : conversion from '__int64 ' to 'unsigned int ', possible loss of data
> C:\Home\brane\src\svn\repo\subversion\libsvn_fs_fs\fs_fs.c(2817) : warning C4244: '=' : conversion from '__int64 ' to 'unsigned int ', possible loss of data
> I think the representation size should be an svn_filesize_t, not an
> unsigned int, especially since it's generated using apr_atoi64 in many
> places.
It's not an unsigned int, it's an apr_size_t, which varies from platform
to platform. It's not like there's an ascii to apr_size_t conversion
function.
But you're right; the size and expanded_size fields of a
representation_t should be svn_filesize_t's, because of what they
represent. I will fix, but I can't find a gcc 3.2.3 option to warn
about implicit type narrowing, so I can't guarantee that I've fixed all
the warnings.
> C:\Home\brane\src\svn\repo\subversion\libsvn_fs_fs\fs_fs.c(3604) : warning C4018: '<=' : signed/unsigned mismatch
> strlen returns a size_t, not an apr_ssize_t.
C is such a bore some times. (int) -1 < (unsigned int) 3 evaluates to
0. In this case, the signed variable is always positive, of course, but
I can see why the warning exists.
> Hmm, and I see you're computing 'strlen(PATH_EXT_TXN)' three times in a
> loop. Premature optimisation may be the root of all evil, but we should
> at least /look/ like we're trying... :-p
I can't imagine this would ever cause a perceptible speed difference,
and therefore I would say it's wrong to compromise the code, even a
little bit, in the name of saving cycles. But in this case the code
also looks a little messy as a result of all those strlen() calls, so I
will change that.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org