On Sat, Dec 17, 2022 at 05:07:12PM +0100, René Scharfe wrote: > > If we are just re-ordering argv, though, it feels like this could still > > work with a strvec. Right now a strvec with "nr" items will free items 0 > > through nr-1, assuming that v[nr] is its NULL invariant. But it could > > free v[nr], too, in case the NULL was swapped into an earlier position. > > > > It's a little weird already, because that swap has violated the > > invariant, so trying to strvec_push() onto it would cause confusing > > results. But if the general use case is to pass the strvec to > > parse_options(), get reordered, and then clear() it, it should work. If > > you wanted to get really fancy, push() et al could double-check the > > invariant and BUG(). > > Yes, parse_options() and strvec are not fitting perfectly. Changing the > former to reorder the elements and keeping them all would require > checking that all callers use the return value. Feels like a lot of work. I think we're already munging the strvec arrays in the option parser, though. I'm just suggesting that parse_options() swap arguments to the end instead of overwriting a NULL (actually, I'm not even sure it doesn't do that already), and strvec_clear() checking the final element. The end state is not necessarily safe, but it's no worse than what we have today. That said... > A variant that takes a strvec and removes and frees parsed items from it > would be clean, but would require refactoring indirect callers like > apply_parse_options() users. Busywork. > > Making a shallow copy to give to parse_options() in callers that currently > pass a strvec directly or indirectly seems like the simplest solution to > me for now. Yes, I thought your original patch actually got to the root of the problem. strvec owns the array and its elements, and parse-options wants to munge the array itself (but not the elements). Making a shallow copy is eliminates the conflict over ownership. -Peff
