On 30.09.2014 15:39, Stefan Fuhrmann wrote: > On Tue, Sep 30, 2014 at 2:40 PM, Branko Čibej <br...@wandisco.com > <mailto:br...@wandisco.com>> wrote: > > On 30.09.2014 13:10, stef...@apache.org > <mailto:stef...@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
