> On Thu, Feb 13, 2014 at 01:29:25PM +0100, Stefan Sperling wrote:
>> On Thu, Feb 13, 2014 at 03:49:46PM +0400, vitalif_at_yourcmc.ru wrote:
>> > --- a/subversion/libsvn_subr/cache-membuffer.c 2014-02-12 21:42:12.483208244 +0000
>> > +++ b/subversion/libsvn_subr/cache-membuffer.c 2014-02-12 21:45:54.275215290 +0000
>> > @@ -1374,7 +1374,9 @@ membuffer_cache_set_internal(svn_membuff
>> > * the old spot, just re-use that space. */
>> > if (entry && ALIGN_VALUE(entry->size) >= size && buffer)
>> > {
>> > - cache->data_used += size - entry->size;
>> > + /* not "+=" because (size - entry_size) is almost always a big 32-bit
>> > + unsigned representation of a negative value on 32-bit platforms */
>> > + cache->data_used = cache->data_used + size - entry->size;
>> > entry->size = size;
>> >
>> > #ifdef SVN_DEBUG_CACHE_MEMBUFFER
>>
>> Style nit: I believe the above comment will be much less clear when
>> read
>> without its preceding context line in the diff, because the reader
>> won't
>> have an obvious way of connecting the reference to "+=" to anything.
>
> How about casting size to apr_uint64_t instead? That allows using the
> previous += since the upcast happens on an operand instead of the
> result
> and is a bit more self-documenting.
>
> cache->data_used += (apr_uint64_t)size - entry->size;
OK, no problem, I agree it's better because it's more explicit :) so the
comment can be removed, yes.
--
With best regards,
Vitaliy Filippov
Received on 2014-02-13 15:42:13 CET