On Tue, Apr 23, 2002 at 03:12:37PM +0200, Sander Striker wrote:
> Hi,
>
> Got lots of warnings on the windows build. This takes care
> of them. Not sure we want to typecast ourselves into oblivion
> though.
>
> This patch deliberately has no log msg (each cast needs review).
>
> Thoughts?
Some of these relate to the issue I filed about adding 'svn_filesize_t'.
Subversion, as an entire system, should be defined to work with 64-bit-sized
files. At least from the API sense. Depending upon the local platform, or
the backend of the FS, or Apache limitations, we might have restrictions
here and there which will drop it. But the *definition* should be 64-bit
throughout.
Note that we only need an unsigned size. If we need canonical values, then
we can simply use 2^64-1, 2^64-2, etc.
>...
> +++ ./subversion/libsvn_fs/strings-table.c Tue Apr 23 13:18:13 2002
> @@ -218,7 +218,7 @@
> svn_fs__clear_dbt (&result);
> result.data = buf;
> result.ulen = *len;
> - result.doff = offset;
> + result.doff = (u_int32_t)offset;
In this case, the 'offset' parameter should be an svn_filesize_t. Thus, this
cast would still be required because we're mapping from the logical space
into an underlying restriction.
>...
> +++ ./subversion/libsvn_fs/reps-strings.c Tue Apr 23 13:18:13 2002
> @@ -445,12 +445,13 @@
> algorithm, and is what has to go when we have a
> true delta combiner. */
> SVN_ERR (rep_read_range (wb->fs, wb->base_rep, sbuf,
> - window->sview_offset, &slen,
> + (apr_size_t)window->sview_offset,
> + &slen,
> wb->trail));
.sview_offset and the parameter to rep_read_range() should be an
svn_filesize_t.
> src_read = 1;
> }
> - memcpy (tbuf + len_read, sbuf + op->offset, op->length);
> - len_read += op->length;
> + memcpy (tbuf + len_read, sbuf + op->offset, (apr_size_t)op->length);
> + len_read += (apr_size_t)op->length;
svn_txdelta_op_t.offset and .length should be an apr_size_t.
>...
> @@ -459,7 +460,7 @@
> /* This could be done in bigger blocks, at the expense
> of some more complexity. */
> int t;
> - for (t = op->offset; t < op->offset + op->length; t++)
> + for (t = (int)op->offset; t < op->offset + op->length; t++)
> tbuf[len_read++] = tbuf[t];
t should be declared as an apr_size_t.
>...
> @@ -468,8 +469,8 @@
> {
> memcpy (tbuf + len_read,
> window->new_data->data + op->offset,
> - op->length);
> - len_read += op->length;
> + (apr_size_t)op->length);
> + len_read += (apr_size_t)op->length;
op->length would be an apr_size_t.
>...
> +++ ./subversion/libsvn_wc/props.c Tue Apr 23 13:10:06 2002
> @@ -659,19 +659,19 @@
> that each .svn subdir remains separable when executing run_log(). */
> str = strstr (base_prop_tmp_path->data, SVN_WC_ADM_DIR_NAME);
> len = base_prop_tmp_path->data + base_prop_tmp_path->len - str;
> - tmp_prop_base = svn_stringbuf_ncreate (str, len, pool);
> + tmp_prop_base = svn_stringbuf_ncreate (str, (apr_size_t)len, pool);
(->data - str) is a ptroff_t. I'm not sure how that maps into a size_t. I
believe the proper sequence here is to declare 'len' as an apr_size_t, cast
the pointer substruction, then add ->len.
That is: one cast still remains, to convert a ptroff_t into an apr_size_t.
> str = strstr (base_propfile_path->data, SVN_WC_ADM_DIR_NAME);
> len = base_propfile_path->data + base_propfile_path->len - str;
> - real_prop_base = svn_stringbuf_ncreate (str, len, pool);
> + real_prop_base = svn_stringbuf_ncreate (str, (apr_size_t)len, pool);
>
> str = strstr (local_prop_tmp_path->data, SVN_WC_ADM_DIR_NAME);
> len = local_prop_tmp_path->data + local_prop_tmp_path->len - str;
> - tmp_props = svn_stringbuf_ncreate (str, len, pool);
> + tmp_props = svn_stringbuf_ncreate (str, (apr_size_t)len, pool);
>
> str = strstr (local_propfile_path->data, SVN_WC_ADM_DIR_NAME);
> len = local_propfile_path->data + local_propfile_path->len - str;
> - real_props = svn_stringbuf_ncreate (str, len, pool);
> + real_props = svn_stringbuf_ncreate (str, (apr_size_t)len, pool);
Same all the way through.
>...
> +++ ./subversion/libsvn_wc/log.c Tue Apr 23 13:24:19 2002
> @@ -1095,7 +1095,7 @@
> apr_hash_this (hi, &key, &keylen, &val);
> entry = val;
>
> - if ((keylen == strlen (SVN_WC_ENTRY_THIS_DIR))
> + if ((keylen == (apr_ssize_t)strlen (SVN_WC_ENTRY_THIS_DIR))
> && (strcmp ((char *) key, SVN_WC_ENTRY_THIS_DIR) == 0))
> is_this_dir = TRUE;
keylen must remain an apr_ssize_t, so this seems fine.
However, the test is rather bogus. SVN_WC_ENTRY_THIS_DIR is scanned twice:
once to get the length, and then once to do a comparison. The whole mess can
be simplified to a simple strcmp(), or the Real Man method:
#define KLEN (sizeof(SVN_WC_ENTRY_THIS_DIR)-1)
is_this_dir = keylen == KLEN
&& memcmp(key, SVN_WC_ENTRY_THIS_DIR, KLEN) == 0;
#undef KLEN
(and remove the =FALSE from the declaration for is_this_dir)
>...
> +++ ./subversion/clients/cmdline/util.c Tue Apr 23 13:24:19 2002
> @@ -463,10 +463,10 @@
> else
> {
> /* Read the entire file into memory. */
> - svn_stringbuf_ensure (new_contents, finfo_after.size + 1);
> + svn_stringbuf_ensure (new_contents, (apr_size_t)(finfo_after.size + 1));
> apr_err = apr_file_read_full (tmp_file,
> new_contents->data,
> - finfo_after.size,
> + (apr_size_t)finfo_after.size,
This is fine. We're reading a file (sized as apr_off_t) into memory (sized
as apr_size_t). Gotta keep the cast.
Strictly, you'd want to check the size of the file, to ensure it isn't
larger than memory capacity. Screw that, however :-)
>...
> +++ ./subversion/libsvn_delta/svndiff.c Tue Apr 23 13:24:19 2002
> @@ -80,7 +80,7 @@
> while (--n >= 0)
> {
> cont = ((n > 0) ? 0x1 : 0x0) << 7;
> - *p++ = ((val >> (n * 7)) & 0x7f) | cont;
> + *p++ = (char)(((val >> (n * 7)) & 0x7f) | cont);
Seems fine.
>...
> @@ -341,10 +341,10 @@
> case svn_txdelta_new:
> if (op.length > new_len - npos)
> return -1;
> - npos += op.length;
> + npos += (apr_size_t)op.length;
op.length should be an apr_size_t, as mentioned before.
>...
> @@ -415,22 +415,22 @@
> p = decode_int (&val, p, end);
> if (p == NULL)
> return SVN_NO_ERROR;
> - sview_len = val;
> + sview_len = (apr_size_t)val;
decode_int() should probably be returning an apr_size_t. It isn't possible
to have a window larger than memory-size. This also maps to what will get
marshalled (values from the delta windows which are mostly apr_size_t
values).
>...
> +++ ./subversion/libsvn_delta/text_delta.c Tue Apr 23 13:18:46 2002
> @@ -155,7 +155,7 @@
> op->action_code = opcode;
> op->offset = bob->new_data->len;
> op->length = length;
> - svn_stringbuf_appendbytes (bob->new_data, new_data, length);
> + svn_stringbuf_appendbytes (bob->new_data, new_data, (apr_size_t)length);
svn_txdelta__insert_op() should be declared with svn_filesize_t and
apr_size_t for its offset/length params.
>...
> @@ -421,7 +421,7 @@
> * overlap to the beginning of the new buffer. */
> if (ab->sbuf_offset + ab->sbuf_len > window->sview_offset)
> {
> - apr_size_t start = window->sview_offset - ab->sbuf_offset;
> + apr_size_t start = (apr_size_t)(window->sview_offset - ab->sbuf_offset);
> memmove (ab->sbuf, old_sbuf + start, ab->sbuf_len - start);
> ab->sbuf_len -= start;
> }
This cast is probably needed. You're taking two svn_filesize_t values,
subtracting them, and generating an offset into the window.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Apr 23 19:57:54 2002