Re: [PATCHv4 6/6] is_submodule_modified(): clear environment properly
- From
Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
- Date
- Feb 24, 2010, 08:55 UTC
- Message-ID
- <cb7bb73a1002240055v58953dcfh3be7c4589523b14b@mail.gmail.com>
- In-Reply-To
- <4B84DDE9.8000301@viscovery.net>
On Wed, Feb 24, 2010 at 9:06 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:
> Giuseppe Bilotta schrieb: >> + const char *env[local_repo_env_size+2]; > > Variable sized arrays are prohibited.
Ah, sorry. Is alloca() allowed? I don't see it being used anywhere else in the code, and malloc would be a little too much for this case.
Show 11 quoted lines
>> struct strbuf buf = STRBUF_INIT; >> >> + /* Copy local_repo_env to env, letting i >> + rest at the last NULL */ >> + while (env[i] = local_repo_env[i]) >> + ++i; /* do nothing */ >> + > > This looks very inconsistent: At the one hand, you use l_r_e_size to > allocate the space, but then you iterate over it assuming that the list is > (also) NULL-terminated. But this is only a minor nit.
Well, if you consider that using l-r-e-size is just a way to spare a double-walk, the inconsistency is tolerable. OTOH, in this particular case NULL-walking the list doesn't give the usual benefit of sparing a counter, so I could rework the patch to use a standard for loop. (I also notice that I forgot to remove the /* do nothing */ comment from the while(env[i] = local_repo_env[i++]) ; --i approach I was going with at first)
-- Giuseppe "Oblomov" Bilotta