On 21.04.2014 19:05, Ivan Zhakov wrote:
> On 21 April 2014 19:49, Stefan Fuhrmann <stefan.fuhrm...@wandisco.com> wrote:
>> On Mon, Apr 21, 2014 at 9:37 AM, Ivan Zhakov <i...@visualsvn.com> wrote:
>>> On 20 April 2014 22:38, Stefan Fuhrmann <stefan.fuhrm...@wandisco.com>
>>> wrote:
>>>> On Sat, Apr 19, 2014 at 10:32 PM, Ivan Zhakov <i...@visualsvn.com>
>>>> wrote:
>>>>> On 25 October 2013 15:12, <stef...@apache.org> wrote:
>>>>>> Author: stefan2
>>>>>> Date: Fri Oct 25 11:12:27 2013
>>>>>> New Revision: 1535686
>>>>>>
> [...]
>>>>>> +/* Determine the checksum for the SIZE bytes of data starting at
>>>>>> START
>>>>>> + * in FILE and return the result in *FNV1_CHECKSUM.
>>>>>> + * Use POOL for tempoary allocations.
>>>>>> + */
>>>>>> +static svn_error_t *
>>>>>> +fnv1a_checksum_on_file_range(apr_uint32_t *fnv1_checksum,
>>>>>> + apr_file_t *file,
>>>>>> + apr_off_t start,
>>>>>> + apr_off_t size,
>>>>>> + apr_pool_t *pool)
>>>>>> +{
>>>>>> + char buffer[4096];
>>>>>> +
>>>>> Why you're using non-standard sized buffer for IO operations on stack?
>>>>> It should be apr_palloc(SVN__STREAM_CHUNK_SIZE). Is not it?
>>>>
>>>> No need to use dynamically alloc the buffer
>>>> but not using SVN__STREAM_CHUNK_SIZE
>>>> as clearly an oversight.
>>>>
>>> Your reply doesn't have any arguments why no need to use buffer from
>>> pool like we do in all other places.
>>
>> You now have the chance to make your statement
>> actually true. Please fix the following occurrences
>> and check their callers for proper pool usage:
>>
>> trunk/subversion/libsvn_delta/text_delta.c:923
>> trunk/subversion/libsvn_subr/io.c:805
>>
> Hi Stefan,
>
> Well, I didn't notice that places. Sorry for confusing you.
>
> It's good time to make consistent in either way.
>
> Where temporary read buffers should be allocated: on stack or in heap
> (pool)? I'm fine with both approaches, but I would like to make it
> consistent. What do you think?
Why do you want the code to be consistent? There are good arguments for
both approaches. Consistency for its own sake makes no sense.
-- Brane
--
Branko Čibej | Director of Subversion
WANdisco // Non-Stop Data
e. br...@wandisco.com
