Am 24.06.2014 14:06, schrieb Tanay Abhra:
> On 6/23/2014 5:25 PM, Matthieu Moy wrote:
>> Tanay Abhra <tanayabh@xxxxxxxxx> writes:
>>
>>> +/* for NULL values, 'util' for each `string_list_item` is flagged as 1 */
>>
>> It's a void *, so just saying that it is flagged as 1 does not say how
>> it's encoded. How about "... is an 'int *' pointing to the value 1".
>>
>> And actually, you can save one malloc by encoding the value directly in
>> the util pointer itself like
>>
>> #define NULL_FLAG (void *)1
>>
>> item->util = NULL_FLAG;
>>
>> I find this a bit ugly, but I think Git already uses this in a few
>> places (git grep 'void \*) *1' for examples).
>>
>> Or you can use a pointer to a single static value:
>>
>> static const int boolean_null_flag = 1; // Or even without = 1.
>>
>> and then
>>
>> item->util = &boolean_null_flag;
>>
>
> Thanks for the review. I will change the flag format to what you have
> suggested. Instead of giving the users of the API the headache of
> thinking about the flag, let me give you an alternative,
>
> +const struct string_list *git_config_get_string_multi(const char *key)
> +{
> + int i, *flag;
> + struct string_list *temp = config_cache_get_value(key);
> + if (!temp)
> + return NULL;
> + /* create a NODUP string list which can have NULL values */
> + struct string_list *l = xcalloc(1, sizeof(*l));
> + for(i=0; i < temp->nr; i++) {
> + flag = temp->items[i].util;
> + if (*flag)
> + string_list_append(l, NULL);
> + else
> + string_list_append(l, temp->items[i].string);
> + }
> + return l;
> +}
>
Returning a list that is 'half-owned' by the caller (i.e. list is, values are not) is kindof strange. Especially weird is that the caller needs to string_list_clear() _and_ free() the list. Why don't you store NULL values in the string_list in the cache instead of doing this flag magic? I.e.
static int config_cache_add_value(const char *key, const char *value)
{
...
if (value == NULL)
string_list_append_nodup(&e->value_list, NULL);
else
string_list_append(&e->value_list, value);
or even
string_list_append_nodup(&e->value_list, value ? xstrdup(value) : NULL);
--
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