On 30.09.2014 15:39, Stefan Fuhrmann wrote:
> On Tue, Sep 30, 2014 at 2:40 PM, Branko Čibej <brane_at_wandisco.com
> <mailto:brane_at_wandisco.com>> wrote:
>
> On 30.09.2014 13:10, stefan2_at_apache.org
> <mailto:stefan2_at_apache.org> wrote:
> > Author: stefan2
> > Date: Tue Sep 30 11:10:02 2014
> > New Revision: 1628393
> >
> > URL: http://svn.apache.org/r1628393
> > Log:
> > Fix / silence some integer conversion warnings.
> >
> > * subversion/libsvn_fs_fs/index.c
> > (packed_stream_read): We know those are well within apr_size_t's
> > value range.
> > (auto_open_l2p_index): The config reader limits the BLOCK_SIZE
> > to apr_size_t since r1628392.
> > (get_l2p_header_body): Use the appropriate type for the iteration
> > variable. Use maximally widening casts
> > for non-negative values where appropriate.
> > Cast revnum count after we verified its
> range
> > to be within MAX_FILES_PER_DIR.
> > (prefetch_l2p_pages): Exit early for non-positive ranges and use
> > maximally widening casts for non-negative
> > values where appropriate.
>
> Uh.
>
> In my book, every cast is wrong,
>
>
> Well, casts are undesirable but not generally wrong.
> I wish I could do with "just int" but that is too large
> for arrays on e.g. x32 and too small for file sizes
> on others, e.g. Windows.
>
> The index code is particularly affected as it needs
> to mediate between in-memory sizes (apr_size_t),
> platform-independent representation (apr_uint64_t)
> and file addressing (apr_off_t).
As I said, it's OK to cast on API boundaries, as long as we can prove
that the casts are safe. SVN<->APR is an API boundary.
>
>
> and widening casts are doubly wrong
> because they're implicit.
>
>
> Yes, they are implicit but VS still warns about signed/
> unsigned conversions. The simplest portable way I can
> think of to make these comparisons work is to cast
> to the largest unsigned we have once we made sure
> that the value is non-negative.
Couldn't care less about VS ... it also warns about a ton of stuff
that's perfectly valid C even when casts are not involved.
The problem is that casts tend to mask actual coding errors from the
compiler. I prefer warnigns from some silly compiler to adding casts
just for the sake of shutting it up.
> Casts are marginally acceptable if they're
> needed to adapt to a poorly designed and/or hardware/platform-specific
> API. But in this case, all the values involved appear to be parameters
> or return values of our own private functions.
>
>
> No. The first one, for instance, converts from apr_off_t
> (for positions in the pack / rev file currently being read)
> to apr_size_t (what the system returns from sizeof).
Red flag right there: what guarantees that a revision or pack file
cannot be larger than 4 gigs, even on 32-bit platforms? One large binary
file (think video assets or chip design files) can easily be larger than
that. I thought we did not have restrictions on file content size.
-- Brane
Received on 2014-09-30 16:02:02 CEST