Am 19.12.2017 um 12:38 schrieb Jeff King: > On Mon, Dec 18, 2017 at 08:18:17PM +0100, René Scharfe wrote: > >>> I'd actually argue the other way: the simplest interface is one where >>> the string list owns all of its pointers. That keeps the >>> ownership/lifetime issues clear, and it's one less step for the caller >>> to have to remember to do at the end (they do have to clear() the list, >>> but they must do that anyway to free the array of items). >>> >>> It does mean that some callers may have to remember to free a temporary >>> buffer right after adding its contents to the list. But that's a lesser >>> evil, I think, since the memory ownership issues are all clearly >>> resolved at the time of add. >>> >>> The big cost is just extra copies/allocations. >> >> An interface requiring callers to allocate can be used to implement a >> wrapper that does all allocations for them -- the other way around is >> harder. It can be used to avoid object duplication, but duplicates >> functions. No idea if that's worth it. > > Sure, but would anybody actually want to _use_ the non-wrapped version? Not sure, but cases that currently use STRING_LIST_INIT_NODUP probably apply. Apropos: apply.c::write_out_results() looks like it might, too. Another question is how much it would cost to let them duplicate strings as well in order to simplify the code. > That's the same duality we have now with string_list. Hmm, I thought we *were* discussing string_list? >>> Having a "format into a string" wrapper doesn't cover _every_ string you >>> might want to add to a list, but my experience with argv_array_pushf >>> leads me to believe that it covers quite a lot of cases. >> >> It would fit in with the rest of the API -- we have string_list_append() >> as a wrapper for string_list_append_nodup()+xstrdup() already. We also >> have similar functions for strbuf and argv_array. I find it a bit sad >> to reimplement xstrfmt() yet again instead of using it directly, though. > > I dunno, I think could provide some safety and some clarity. IOW, why > _don't_ we like: > > string_list_append_nodup(list, xstrfmt(fmt, ...)); > > ? I think because: > > 1. It's a bit long and ugly. > > 2. It requires a magic "nodup", because we're violating the memory > ownership boundary. > > 3. It doesn't provide any safety for the case where strdup_strings is > not set, making it easy to leak accidentally. Right, and at least 2 and 3 would be solved by having distinct types for the plain and the duplicating variants. The plain one would always "nodup" and would have no flags that need to be checked. > Doing: > > string_list_appendf(list, fmt, ...); > > pushes the memory ownership semantics "under the hood" of the > string_list API. And as opposed to being a simple wrapper, it could > assert that strdup_strings is set (we already do some similar assertions > in the split functions). Yes, that check would guard against leaks. Having few functions that can be combined looks like a cleaner interface to me than having additional shortcuts for specific combinations -- less duplication, smaller surface. That said I'm not against adding string_list_appendf(); we already have similar functions for other types. René
