From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Fri, 28 Feb 2014 14:20:09 GMT
Subject: Re: [PATCH v2 01/11] builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW()
Message-ID: <53109B19.8070103@alum.mit.edu>
In-Reply-To: <CACsJy8AmQeVb-i6Sn0BH-_ikEXPHTdtKnQRyzChX1WfD9Lj4Jw@mail.gmail.com>

On 02/28/2014 01:40 PM, Duy Nguyen wrote:
> On Fri, Feb 28, 2014 at 7:32 PM, Duy Nguyen <pclouds@gmail.com> wrote:
>>>         done_pbase_paths_num++;
>>
>> If you move this up one line, then you don't have to "+ 1" in ALLOC_GROW
>>
> 
> same comment to a few other patches. The rest of your series looks good.

Duy,

The example in Documentation/technical/api-allocation-growing.txt does
it the same way as Dmitry:

    ALLOC_GROW(item, nr + 1, alloc);
    item[nr++] = value you like;

The alternative,

    nr++;
    ALLOC_GROW(item, nr, alloc);
    item[nr] = value you like;

is an extra line, which is at least a small argument for the variant
shown in the docs.  (Since ALLOC_GROW is a macro, it is not OK to use
"++nr" as its second argument.)  Personally, I also prefer the shorter
version.  The line

    item[nr++] = value

is an easy-to-recognize idiom, and

    ALLOC_GROW(item, nr + 1, alloc);

somehow makes it more transparent by how much more space will be needed.

So my vote is that the patches are OK the way Dmitry wrote them (mind, I
have only read through 05/11 so far).

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/