Re: [PATCH v2 2/4] xdiff/xprepare: refactor abort cleanups
- From
Phil Hord <hordp@cisco.com>
- Date
- Jul 7, 2011, 03:55 UTC
- Message-ID
- <4E152E31.4020105@cisco.com>
- In-Reply-To
- <7vaacrhtp3.fsf@alter.siamese.dyndns.org>
On 07/06/2011 07:31 PM, Junio C Hamano wrote:
Show 60 quoted lines
> Tay Ray Chuan <rctay89@gmail.com> writes:
>
>> On Thu, Jul 7, 2011 at 1:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Tay Ray Chuan <rctay89@gmail.com> writes:
>>>
>>>> Group free()'s that are called when a malloc() fails in
>>>> xdl_prepare_ctx(), making for more readable code.
>>> Nicer, but I wonder if we can get away with a single label that lets us
>>> abort with freeing whatever we have allocated by making sure that the
>>> various pointer fields and variables are initialized to NULL, which may
>>> lead to even more readable code.
>> Pardon my dullness. Do you mean to check if the various fields are set
>> to NULL upon allocation, and free()'ing them if so?
> What I meant was that, instead of doing this:
>
> func() {
> if (somefunc(&A, ...) < 0)
> goto failA;
> ... do something ...
> B = someotheralloc();
> if (!B)
> goto failB;
> ... do something ...
> C = yetanotheralloc();
> if (!C)
> goto failC;
> ... do things using A, B, C ...
>
> return 0;
> failC:
> free(B);
> failB:
> free(A);
> failA:
> return -1;
> }
>
> it would be easier to follow if you did:
>
> func() {
> A = B = C = NULL;
> if (somefunc(&A, ...) < 0)
> goto fail;
> ... do something ...
> B = someotheralloc();
> if (!B)
> goto fail;
> ... do something ...
> C = yetanotheralloc();
> if (!C)
> goto fail;
> ... do things using A, B, C ...
>
> fail:
> free(B);
> free(A);
> return -1;
> }
>
> Especially when you have more than 2 such fail labels.Because 'free(NULL)' is handled sanely (as a no-op). http://pubs.opengroup.org/onlinepubs/009695399/functions/free.html
I haven't looked, but I assume xdl_cha_free does the same thing. I only assume this because Junio seems to imply it, though.
Phil