Re: [PATCH 0/11] renaming argv_array

[Jeff King <peff@peff.net>]

On Wed, Jul 29, 2020 at 08:15:54AM +0200, Christian Couder wrote:

> On Tue, Jul 28, 2020 at 10:23 PM Jeff King <peff@peff.net> wrote:
> >
> > The argv_array data type has turned out to be useful in our code base,
> > but the name isn't very good. From patch 2 of this series:
> >
> >   The name "argv-array" isn't very good, because it describes what the
> >   data type can be used for (program argument arrays), not what it
> >   actually is (a dynamically-growing string array that maintains a
> >   NULL-terminator invariant).
> 
> I cannot help but notice that you still use "array" when describing
> what it is. You actually use "string array" to describe what it is,
> and at the same time say that the name should describe what it is. So
> I would expect after the above sentence that you would rename it to
> "string_array" or "str_array".

I think one thing that leads me to "vec" is the "v" from "argv", which
implies the NULL-terminator (though of course that's _not_ implied by
"vector" in other languages). None of the other "array" types have that
feature, and it's an important one here.

> > "strarray" would work, too, but it's
> > longer and a bit more awkward to say (and don't we all say these things
> > in our mind as we type them?).
> 
> It's longer than "strarray" by 2 characters only.

Yeah, I agree the length between the two is not super important. Mostly
I find it harder to read and say. That's obviously subjective, but I do
think counts for something.

We use an underscore for "oid_array" (which I think makes sense because
it's awkward to concatenate directly a word that starts with a vowel and
has multiple syllables). That makes it inconsistent with oidmap and
oidset. Likewise, I was hoping for consistency with strbuf here.

> Also we still use "array" in "oid_array" which is very similar to
> this. And the implementation is based on the ALLOC_GROW macro which
> uses the REALLOC_ARRAY macro.
> 
> We also use ALLOC_ARRAY, FLEX_ARRAY, CALLOC_ARRAY, COPY_ARRAY and
> MOVE_ARRAY macros.

Those macro names are pretty bulky, but I think it matters a lot less
because they're used a lot less. The type name here is prepended to all
of the "method" functions like strvec_pushf(), etc.

> So if you don't like the "array" part of the name, are you going to
> also change "oid_array" into "oidvec" and for example "REALLOC_ARRAY"
> into "REALLOC_VEC" or "REALLOC_VECTOR"?

I hadn't planned on it, just because I think the cost of changing versus
the benefit is not that high. If I were designing from scratch, I'd
definitely consider oidvec (but probably not REALLOC_VEC for the reasons
above).

I think the cost of changing away from argv_array _is_ worth it, and
once we're doing that, we can choose between the alternatives without
paying an extra cost.

> If you want to change only "argv_array" (and not also "oid_array",
> "REALLOC_ARRAY" and perhaps other *_ARRAY macros) into something else,
> then I think it would be better to be consistent with them.

I definitely sympathize with the consistency argument, and I don't think
'str_array" is the end of the world. But I think there are many
dimensions of inconsistency already, so without renaming every data
structure, we're going to be inconsistent with something. Mostly I just
subjectively find strvec easier to read, say, and type, and I don't
think the inconsistencies are so glaring that it's a problem.

-Peff