Re: [PATCH v3 2/3] config: add hashtable for config parsing & retrieval

Am 24.06.2014 14:06, schrieb Tanay Abhra:

On 6/23/2014 5:25 PM, Matthieu Moy wrote:

quoted

Tanay Abhra [off-list ref] writes:

quoted

+/* 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);