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