Re: Re*: [PATCH v4 3/4] string-list: provide `string_list_appendf()`

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, May 21, 2018 at 09:01:05AM +0900, Junio C Hamano wrote:

> 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.

IMHO such a mixed use is mildly crazy. At any rate, we would know that
anybody using appendf() would not have this problem, since we are just
introducing it now.

> 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).

I do agree that argv_array is generally a better fit for most cases.
Didn't we want to rename it to strarray or something? That's probably
too much yak-shaving for this series, though. :)

> We would need to make _pushf() and friends return "const char *" if
> we go that route to make the resulting API more useful, though.

This is the first time I think that's been suggested, but I agree it's
the only sensible thing for the functions to return.

> -- >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>
> ---

Yup, this looks good to me.

-Peff



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux