[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1628393 - /subversion/trunk/subversion/libsvn_fs_fs/index.c

From: Stefan Fuhrmann <stefan.fuhrmann_at_wandisco.com>
Date: Tue, 30 Sep 2014 15:39:22 +0200

On Tue, Sep 30, 2014 at 2:40 PM, Branko Čibej <brane_at_wandisco.com> wrote:

> On 30.09.2014 13:10, 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).

> 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.
Received on 2014-09-30 15:39:51 CEST

This is an archived mail posted to the Subversion Dev mailing list.