Jacob Keller <jacob.keller@xxxxxxxxx> writes:
> On Sun, May 20, 2018 at 3:17 AM, Martin Ågren <martin.agren@xxxxxxxxx> wrote:
>> +/**
>> + * Add formatted string to the end of `list`. This function ignores
>> + * the value of `list->strdup_strings` and always appends a freshly
>> + * allocated string, so you will probably not want to use it with
>> + * `strdup_strings = 0`.
>> + */
>> +struct string_list_item *string_list_appendf(struct string_list *list,
>> + const char *fmt, ...);
>> +
>
> Would it make sense to verify that strdup_strings == 0? I guess we'd
> have to use die or BUG(), but that would mean that the program could
> crash..
It probably is clear to readers that any reasonable implementation
of *_appendf() will create a new and unique string, as the point of
*f() is to give a customized instantiation of fmt string for given
parameters. So it would be natural to expect that the storage that
holds the generated string will belong to the list. We _could_ make
it honor strdup_strings and make one extra copy when strdup_strings
is set to true, but the only effect such a stupid implementation has
is to unnecessarily leak ;-)
I think it is probably OK to check and BUG() when strdup_strings==0,
but such a check means that we now declare that a string list must
either borrow all of its strings from elsewhere or own all of its
strings itself, and mixture is not allowed.
The (overly) flexible string_list API could be used to mix both
borrowed and owned strings (an obvious strategy to do this without
leaking and crashing is to use the .util field to mark which ones
are owned and which ones are borrowed), so there might already be
current users of the API that violates that rule.
I have a feeling that argv_array might be a better fit for the
purpose of keeping track of to_free[] strings in the context of this
series. Moving away from string_list would allow us to sidestep the
storage ownership issues the API has, and we do not need the .util
thing string_list gives us (which is one distinct advantage string_list
has over argv_array, if the application needs that feature).
We would need to make _pushf() and friends return "const char *" if
we go that route to make the resulting API more useful, though.
-- >8 --
Subject: argv-array: return the pushed string from argv_push*()
Such an API change allows us to use an argv_array this way:
struct argv_array to_free = ARGV_ARRAY_INIT;
const char *msg;
if (some condition) {
msg = "constant string message";
... other logic ...
} else {
msg = argv_pushf(&to_free, "format %s", var);
}
... use "msg" ...
... do other things ...
argv_clear(&to_free);
Note that argv_array_pushl() and argv_array_pushv() are used to push
one or more strings with a single call, so we do not return any one
of these strings from these two functions in order to reduce the
chance to misuse the API.
Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
---
argv-array.c | 6 ++++--
argv-array.h | 4 ++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/argv-array.c b/argv-array.c
index 5d370fa336..449dfc105a 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -21,12 +21,13 @@ static void argv_array_push_nodup(struct argv_array *array, const char *value)
array->argv[array->argc] = NULL;
}
-void argv_array_push(struct argv_array *array, const char *value)
+const char *argv_array_push(struct argv_array *array, const char *value)
{
argv_array_push_nodup(array, xstrdup(value));
+ return array->argv[array->argc - 1];
}
-void argv_array_pushf(struct argv_array *array, const char *fmt, ...)
+const char *argv_array_pushf(struct argv_array *array, const char *fmt, ...)
{
va_list ap;
struct strbuf v = STRBUF_INIT;
@@ -36,6 +37,7 @@ void argv_array_pushf(struct argv_array *array, const char *fmt, ...)
va_end(ap);
argv_array_push_nodup(array, strbuf_detach(&v, NULL));
+ return array->argv[array->argc - 1];
}
void argv_array_pushl(struct argv_array *array, ...)
diff --git a/argv-array.h b/argv-array.h
index 29056e49a1..715c93b246 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -12,9 +12,9 @@ struct argv_array {
#define ARGV_ARRAY_INIT { empty_argv, 0, 0 }
void argv_array_init(struct argv_array *);
-void argv_array_push(struct argv_array *, const char *);
+const char *argv_array_push(struct argv_array *, const char *);
__attribute__((format (printf,2,3)))
-void argv_array_pushf(struct argv_array *, const char *fmt, ...);
+const char *argv_array_pushf(struct argv_array *, const char *fmt, ...);
LAST_ARG_MUST_BE_NULL
void argv_array_pushl(struct argv_array *, ...);
void argv_array_pushv(struct argv_array *, const char **);
