Tanay Abhra [off-list ref] 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;
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
On 6/23/2014 5:25 PM, Matthieu Moy wrote:
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;
+}
Now the only headache for the user will be to free the string-list once
he is done with it. What do you think about this approach?
Also I have a doubt, when rewriting git_config() callers I saw a
curious case, when at the end of the callback they sometimes call
git_default_config().
For example in add.c,
static int add_config(const char *var, const char *value, void *cb)
{
if (!strcmp(var, "add.ignoreerrors") ||
!strcmp(var, "add.ignore-errors")) {
ignore_add_errors = git_config_bool(var, value);
return 0;
}
return git_default_config(var, value, cb);
}
What should I do about this, should I change the default_config() calls
to git_config_get_string calls which I can easily do, if the values it
is setting are not being set by any other callers.
For example in config.c,
static int git_default_core_config(const char *var, const char *value)
{
/* This needs a better name */
if (!strcmp(var, "core.filemode")) {
trust_executable_bit = git_config_bool(var, value);
return 0;
}
if (!strcmp(var, "core.trustctime")) {
trust_ctime = git_config_bool(var, value);
return 0;
}
if (!strcmp(var, "core.statinfo") ||
!strcmp(var, "core.checkstat")) {
--[snip]
variables like "core.filemode" or "core.statinfo" have been requested
here only in the whole codebase, so I am safe to rewrite this function
with git_config_get_string, am I?
If not many callers (add_config...) behave like this. What should I
do with them?
Also, what are your views for the points Ramsay has raised.
Cheers,
Tanay Abhra.
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);