On Thu, Feb 13, 2014 at 01:29:25PM +0100, Stefan Sperling wrote:
On Thu, Feb 13, 2014 at 03:49:46PM +0400, vita...@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
