{"thread":{"id":"60734","subject":"[PATCH] strvec: use correct member name in comments","startedAt":"2024-01-12T07:06:39Z","lastAt":"2024-01-14T18:20:05Z","messageCount":7,"participants":["Linus Arver via GitGitGadget","Jeff King","Linus Arver","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"486689","messageId":"pull.1640.git.1705043195997.gitgitgadget@gmail.com","threadId":"60734","inReplyTo":null,"subject":"[PATCH] strvec: use correct member name in comments","fromName":"Linus Arver via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-01-12T07:06:35Z","receivedAt":"2024-01-12T07:06:39Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"From: Linus Arver \n\nIn d70a9eb611 (strvec: rename struct fields, 2020-07-28), we renamed the\n\"argv\" member to \"v\". In the same patch we also did the following rename\nin strvec.c:\n\n -void strvec_pushv(struct strvec *array, const char **argv)\n +void strvec_pushv(struct strvec *array, const char **items)\n\nand it appears that this s/argv/items operation was erroneously applied\nto strvec.h.\n\nRename \"items\" to \"v\".\n\nSigned-off-by: Linus Arver \n---\n strvec: use correct member name in comments\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1640%2Flistx%2Ffix-strvec-typos-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1640/listx/fix-strvec-typos-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1640\n\n strvec.h | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/strvec.h b/strvec.h\nindex 9f55c8766ba..4715d3e51f8 100644\n--- a/strvec.h\n+++ b/strvec.h\n@@ -4,8 +4,8 @@\n /**\n * The strvec API allows one to dynamically build and store\n * NULL-terminated arrays of strings. A strvec maintains the invariant that the\n- * `items` member always points to a non-NULL array, and that the array is\n- * always NULL-terminated at the element pointed to by `items[nr]`. This\n+ * `v` member always points to a non-NULL array, and that the array is\n+ * always NULL-terminated at the element pointed to by `v[nr]`. This\n * makes the result suitable for passing to functions expecting to receive\n * argv from main().\n *\n@@ -22,7 +22,7 @@ extern const char *empty_strvec[];\n \n /**\n * A single array. This should be initialized by assignment from\n- * `STRVEC_INIT`, or by calling `strvec_init`. The `items`\n+ * `STRVEC_INIT`, or by calling `strvec_init`. The `v`\n * member contains the actual array; the `nr` member contains the\n * number of elements in the array, not including the terminating\n * NULL.\n@@ -80,7 +80,7 @@ void strvec_split(struct strvec *, const char *);\n void strvec_clear(struct strvec *);\n \n /**\n- * Disconnect the `items` member from the `strvec` struct and\n+ * Disconnect the `v` member from the `strvec` struct and\n * return it. The caller is responsible for freeing the memory used\n * by the array, and by the strings it references. After detaching,\n * the `strvec` is in a reinitialized state and can be pushed\n\nbase-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b\n-- \ngitgitgadget\n"},{"id":"486693","messageId":"20240112074138.GH618729@coredump.intra.peff.net","threadId":"60734","inReplyTo":"pull.1640.git.1705043195997.gitgitgadget@gmail.com","subject":"Re: [PATCH] strvec: use correct member name in comments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-12T07:41:38Z","receivedAt":"2024-01-12T07:41:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 12, 2024 at 07:06:35AM +0000, Linus Arver via GitGitGadget wrote:\n\n> From: Linus Arver \n> \n> In d70a9eb611 (strvec: rename struct fields, 2020-07-28), we renamed the\n> \"argv\" member to \"v\". In the same patch we also did the following rename\n> in strvec.c:\n> \n> -void strvec_pushv(struct strvec *array, const char **argv)\n> +void strvec_pushv(struct strvec *array, const char **items)\n> \n> and it appears that this s/argv/items operation was erroneously applied\n> to strvec.h.\n> \n> Rename \"items\" to \"v\".\n\nGood catch. The source of the problem is that the patch originally used\n\"items\" in the struct, too, but after review we settled on the more\nconcise \"v\". I'd almost certainly have then flipped the name in the\nstruct definition and relied on the compiler to help find the fallout.\nBut of course it doesn't look in comments. :)\n\nAs you note, we still call use \"items\" for the vector passed in to\npushv. I think that is OK, and there is no real need to use the terse\n\"v\" there (it is also purely internal; the declaration in strvec.h does\nnot name it at all).\n\nSo this patch looks great to me. Thanks!\n\n-Peff\n"},{"id":"486722","messageId":"owlyo7dqig1w.fsf@fine.c.googlers.com","threadId":"60734","inReplyTo":"20240112074138.GH618729@coredump.intra.peff.net","subject":"Re: [PATCH] strvec: use correct member name in comments","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-01-12T18:04:43Z","receivedAt":"2024-01-12T18:04:45Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Jeff King writes:\n\n> The source of the problem is that the patch originally used\n> \"items\" in the struct, too\n\nAh, that makes sense.\n\n> As you note, we still call use \"items\" for the vector passed in to\n> pushv. I think that is OK, and there is no real need to use the terse\n> \"v\" there (it is also purely internal; the declaration in strvec.h does\n> not name it at all).\n\nIndeed. Perhaps I should have included this in my commit message.\n\nSide note: should we start naming the parameters in strvec.h? I would\nthink that it wouldn't hurt at this point (as the API is pretty stable).\nIf you think that's worth it, I could reroll to include that in this\nseries (and also improve my commit message for this patch).\n"},{"id":"486737","messageId":"xmqqjzoe8br0.fsf@gitster.g","threadId":"60734","inReplyTo":"owlyo7dqig1w.fsf@fine.c.googlers.com","subject":"Re: [PATCH] strvec: use correct member name in comments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-12T21:47:47Z","receivedAt":"2024-01-12T21:47:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Arver writes:\n\n> Side note: should we start naming the parameters in strvec.h? I would\n> think that it wouldn't hurt at this point (as the API is pretty stable).\n> If you think that's worth it, I could reroll to include that in this\n> series (and also improve my commit message for this patch).\n\nI am not sure if it adds more value to outweigh the cost of\nchurning. When the meaning of the parameters are obvious only by\nlooking at their types, a prototype without parameter names is\neasier to maintain, by allowing the parameters to be renamed only\nonce in the implementation. When the meaning of parameters are not\nobvious from their types, we do want them to be named so that you\nonly have to refer to the header files to know the argument order.\n\n\"void *calloc(size_t, size_t)\" would not tell us if we should pass\nthe size of individual element or the number of elements first, and\nwriting \"void *calloc(size_t nmemb, size_t size)\" to make it more\nobvious is a good idea.\n\nOn the other hand, \"void *realloc(void *, size_t)\" is sufficient to\ntell us that we are passing a pointer as the first parameter and the\ndesired size as the second parameter, without them having any name.\n\nAre there functions declared in strvec.h you have in mind that their\nparameters are confusing and hard to guess what they mean? \n\nThanks.\n"},{"id":"486747","messageId":"owlyle8uhxut.fsf@fine.c.googlers.com","threadId":"60734","inReplyTo":"xmqqjzoe8br0.fsf@gitster.g","subject":"Re: [PATCH] strvec: use correct member name in comments","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-01-13T00:37:46Z","receivedAt":"2024-01-13T00:37:48Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Junio C Hamano writes:\n\n> Linus Arver writes:\n>\n>> Side note: should we start naming the parameters in strvec.h? I would\n>> think that it wouldn't hurt at this point (as the API is pretty stable).\n>> If you think that's worth it, I could reroll to include that in this\n>> series (and also improve my commit message for this patch).\n>\n> I am not sure if it adds more value to outweigh the cost of\n> churning. When the meaning of the parameters are obvious only by\n> looking at their types, a prototype without parameter names is\n> easier to maintain, by allowing the parameters to be renamed only\n> once in the implementation. When the meaning of parameters are not\n> obvious from their types, we do want them to be named so that you\n> only have to refer to the header files to know the argument order.\n\nThis sounds like a good rule to me.\n\n> \"void *calloc(size_t, size_t)\" would not tell us if we should pass\n> the size of individual element or the number of elements first, and\n> writing \"void *calloc(size_t nmemb, size_t size)\" to make it more\n> obvious is a good idea.\n>\n> On the other hand, \"void *realloc(void *, size_t)\" is sufficient to\n> tell us that we are passing a pointer as the first parameter and the\n> desired size as the second parameter, without them having any name.\n\nThanks for the illuminating examples. Agreed.\n\n> Are there functions declared in strvec.h you have in mind that their\n> parameters are confusing and hard to guess what they mean?\n\nTBH I only learned recently (while writing the patch in this thread)\nthat parameter names in prototypes were optional. I got a little\nconfused initially when looking at strvec.h for the first time because\nnone of the parameters were named. Having thought a bit more about these\nfunctions, none of them have repeated types like in your example where\nnaming is warranted, so I think they're fine as is.\n\nOTOH if we were treating these .h files as something meant for direct\nexternal consumption (that is, if strvec.h is libified and external\nusers outside of Git are expected to use it directly as their first\npoint of documentation), at that point it might make sense to name the\nparameters (akin to the style of manpages for syscalls). But I imagine\nat that point we would have some other means of developer docs (beyond\nraw header files) for libified parts of Git, so even in that case it's\nprobably fine to keep things as is.\n\nThanks.\n"},{"id":"486760","messageId":"20240113073131.GA657764@coredump.intra.peff.net","threadId":"60734","inReplyTo":"owlyle8uhxut.fsf@fine.c.googlers.com","subject":"Re: [PATCH] strvec: use correct member name in comments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-01-13T07:31:31Z","receivedAt":"2024-01-13T07:31:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 12, 2024 at 04:37:46PM -0800, Linus Arver wrote:\n\n> OTOH if we were treating these .h files as something meant for direct\n> external consumption (that is, if strvec.h is libified and external\n> users outside of Git are expected to use it directly as their first\n> point of documentation), at that point it might make sense to name the\n> parameters (akin to the style of manpages for syscalls). But I imagine\n> at that point we would have some other means of developer docs (beyond\n> raw header files) for libified parts of Git, so even in that case it's\n> probably fine to keep things as is.\n\nI think this is mostly orthogonal to libification. Whether the audience\nis other parts of Git or users outside of Git, they need to know how to\ncall the function. Our main source of documentation there is comments\nabove the declaration (we've marked these with \"/**\" which would allow a\nparser to pull them into a separate doc file, but AFAIK in the 9 years\nsince we started that convention, nobody has bothered to write such a\nscript).\n\nNaming the parameters can help when writing those comments, because you\ncan then refer to them (e.g., see the comment above strbuf_addftime).\nEven without that, I think they can be helpful, but I don't think I'd\nbother adding them in unless taking a pass over the whole file, looking\nfor comments that do not sufficiently explain their matching functions.\n\nI don't doubt that some of that would be necessary for libification,\njust to increase the quality of the documentation. But I think it's\nlargely separate from the patch in this thread.\n\n-Peff\n"},{"id":"486777","messageId":"owlyfryzixpo.fsf@fine.c.googlers.com","threadId":"60734","inReplyTo":"20240113073131.GA657764@coredump.intra.peff.net","subject":"Re: [PATCH] strvec: use correct member name in comments","fromName":"Linus Arver","fromEmail":"linusa@google.com","sentAt":"2024-01-14T18:20:03Z","receivedAt":"2024-01-14T18:20:05Z","isPatch":true,"sender":{"key":"linus@ucla.edu","avatar":null},"body":"Jeff King writes:\n\n> On Fri, Jan 12, 2024 at 04:37:46PM -0800, Linus Arver wrote:\n>\n>> OTOH if we were treating these .h files as something meant for direct\n>> external consumption (that is, if strvec.h is libified and external\n>> users outside of Git are expected to use it directly as their first\n>> point of documentation), at that point it might make sense to name the\n>> parameters (akin to the style of manpages for syscalls). But I imagine\n>> at that point we would have some other means of developer docs (beyond\n>> raw header files) for libified parts of Git, so even in that case it's\n>> probably fine to keep things as is.\n>\n> I think this is mostly orthogonal to libification. Whether the audience\n> is other parts of Git or users outside of Git, they need to know how to\n> call the function. Our main source of documentation there is comments\n> above the declaration (we've marked these with \"/**\" which would allow a\n> parser to pull them into a separate doc file, but AFAIK in the 9 years\n> since we started that convention, nobody has bothered to write such a\n> script).\n>\n> Naming the parameters can help when writing those comments, because you\n> can then refer to them (e.g., see the comment above strbuf_addftime).\n> Even without that, I think they can be helpful, but I don't think I'd\n> bother adding them in unless taking a pass over the whole file, looking\n> for comments that do not sufficiently explain their matching functions.\n\nSo in summary you are saying that the comments are the most important\nsource of documentation that we have currently, and unless naming the\nparameters improves these comments, we shouldn't bother naming these\nparameters. I agree.\n\n> I don't doubt that some of that would be necessary for libification,\n> just to increase the quality of the documentation. But I think it's\n> largely separate from the patch in this thread.\n\nI agree with both statements. Thanks.\n"}]}