On 12/12/2011 07:07 PM, Junio C Hamano wrote:
> Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx>
>
> Thanks.
>
> The patch looks correct but I have a slight maintainability concern and a
> suggestion for possible improvement.
>
>> ...
>> diff --git a/builtin/checkout.c b/builtin/checkout.c
>> index b7c6302..a66d3eb 100644
>> --- a/builtin/checkout.c
>> +++ b/builtin/checkout.c
>> @@ -696,17 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
>> {
>> int ret = 0;
>> struct branch_info old;
>> + char *path;
>> unsigned char rev[20];
>> int flag;
>> memset(&old, 0, sizeof(old));
>> - old.path = resolve_ref("HEAD", rev, 0, &flag);
>> - if (old.path)
>> - old.path = xstrdup(old.path);
>> + old.path = path = resolve_refdup("HEAD", rev, 0, &flag);
>
> This uses "one 'const char *' pointer that is used for reading data from
> and an extra 'char *' pointer that is used only for freeing" approach,
> which has two advantages and one disadvantage:
> [...]
> When naming a "for-freeing" pointer variable, the kind of data the area of
> memory happens to contain is of secondary importance. Being deliberately
> vague about what the area of memory may contain is a good thing, because
> it actively discourages the program from looking at the area via the
> pointer if the variable is named "to_free" or something that does not
> specify what it contains.
The to_free variable could even be declared void* to make it even less
(ab)usable.
Michael
--
Michael Haggerty
mhagger@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html