Re: [PATCH v2 01/11] builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW()
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Feb 28, 2014, 14:36 UTC
- Message-ID
- <CACsJy8C1Jv-7Fz=qTZ94vZCVD6s39iju2BRZdhnkSR25VoJ=Ow@mail.gmail.com>
- In-Reply-To
- <53109B19.8070103@alum.mit.edu>
On Fri, Feb 28, 2014 at 9:20 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 29 quoted lines
> 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).
I'm not saying all patches should do
nr++; ALLOC_GROW(item, nr, alloc);
only those that do
if (..) realloc...; nr++; ....
should be reordered. Those changes that do item[nr++] = yyy should be kept. Anyway it's just an observation, not something that should block these patches.
-- Duy