On Tue, Sep 30, 2014 at 2:40 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 30.09.2014 13:10, 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). > 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. > 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). The wiggle room we do have is with the data types we use in our structs. The options are: * Use platform-specific types. Then, we may need to convert at the interfaces (files, network and API). * Use SVN-specific types of a fixed size. Then we need to convert when calling into libs / OS. Traditionally, SVN tends to follow the first approach and has seen some problems with 'long' as revision number and 'int' as array index. For some parts of the FSFS index and more so in FSX, I'm following the second approach. Not sure which one is actually better in the long run. -- Stefan^2.
