strvec: use correct member name in comments #1640
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In d70a9eb (strvec: rename struct fields, 2020-07-28), we renamed the "argv" member to "v". In the same patch we also did the following rename in strvec.c: -void strvec_pushv(struct strvec *array, const char **argv) +void strvec_pushv(struct strvec *array, const char **items) and it appears that this s/argv/items operation was erroneously applied to strvec.h. Rename "items" to "v". Signed-off-by: Linus Arver <linusa@google.com>
listx
force-pushed
the
fix-strvec-typos
branch
from
95497e8 to
8bfc8ca
Compare
|
/preview |
|
Preview email sent as pull.1640.git.1705041969928.gitgitgadget@gmail.com |
|
/preview |
|
Preview email sent as pull.1640.git.1705042170325.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.1640.git.1705043195997.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Jeff King wrote (reply to this): On Fri, Jan 12, 2024 at 07:06:35AM +0000, Linus Arver via GitGitGadget wrote:
> From: Linus Arver <linusa@google.com>
>
> In d70a9eb611 (strvec: rename struct fields, 2020-07-28), we renamed the
> "argv" member to "v". In the same patch we also did the following rename
> in strvec.c:
>
> -void strvec_pushv(struct strvec *array, const char **argv)
> +void strvec_pushv(struct strvec *array, const char **items)
>
> and it appears that this s/argv/items operation was erroneously applied
> to strvec.h.
>
> Rename "items" to "v".
Good catch. The source of the problem is that the patch originally used
"items" in the struct, too, but after review we settled on the more
concise "v". I'd almost certainly have then flipped the name in the
struct definition and relied on the compiler to help find the fallout.
But of course it doesn't look in comments. :)
As you note, we still call use "items" for the vector passed in to
pushv. I think that is OK, and there is no real need to use the terse
"v" there (it is also purely internal; the declaration in strvec.h does
not name it at all).
So this patch looks great to me. Thanks!
-Peff |
|
User |
|
On the Git mailing list, Linus Arver wrote (reply to this): Jeff King <peff@peff.net> writes:
> The source of the problem is that the patch originally used
> "items" in the struct, too
Ah, that makes sense.
> As you note, we still call use "items" for the vector passed in to
> pushv. I think that is OK, and there is no real need to use the terse
> "v" there (it is also purely internal; the declaration in strvec.h does
> not name it at all).
Indeed. Perhaps I should have included this in my commit message.
Side note: should we start naming the parameters in strvec.h? I would
think that it wouldn't hurt at this point (as the API is pretty stable).
If you think that's worth it, I could reroll to include that in this
series (and also improve my commit message for this patch). |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Linus Arver <linusa@google.com> writes:
> Side note: should we start naming the parameters in strvec.h? I would
> think that it wouldn't hurt at this point (as the API is pretty stable).
> If you think that's worth it, I could reroll to include that in this
> series (and also improve my commit message for this patch).
I am not sure if it adds more value to outweigh the cost of
churning. When the meaning of the parameters are obvious only by
looking at their types, a prototype without parameter names is
easier to maintain, by allowing the parameters to be renamed only
once in the implementation. When the meaning of parameters are not
obvious from their types, we do want them to be named so that you
only have to refer to the header files to know the argument order.
"void *calloc(size_t, size_t)" would not tell us if we should pass
the size of individual element or the number of elements first, and
writing "void *calloc(size_t nmemb, size_t size)" to make it more
obvious is a good idea.
On the other hand, "void *realloc(void *, size_t)" is sufficient to
tell us that we are passing a pointer as the first parameter and the
desired size as the second parameter, without them having any name.
Are there functions declared in strvec.h you have in mind that their
parameters are confusing and hard to guess what they mean?
Thanks. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@83a49ac. |
|
On the Git mailing list, Linus Arver wrote (reply to this): Junio C Hamano <gitster@pobox.com> writes:
> Linus Arver <linusa@google.com> writes:
>
>> Side note: should we start naming the parameters in strvec.h? I would
>> think that it wouldn't hurt at this point (as the API is pretty stable).
>> If you think that's worth it, I could reroll to include that in this
>> series (and also improve my commit message for this patch).
>
> I am not sure if it adds more value to outweigh the cost of
> churning. When the meaning of the parameters are obvious only by
> looking at their types, a prototype without parameter names is
> easier to maintain, by allowing the parameters to be renamed only
> once in the implementation. When the meaning of parameters are not
> obvious from their types, we do want them to be named so that you
> only have to refer to the header files to know the argument order.
This sounds like a good rule to me.
> "void *calloc(size_t, size_t)" would not tell us if we should pass
> the size of individual element or the number of elements first, and
> writing "void *calloc(size_t nmemb, size_t size)" to make it more
> obvious is a good idea.
>
> On the other hand, "void *realloc(void *, size_t)" is sufficient to
> tell us that we are passing a pointer as the first parameter and the
> desired size as the second parameter, without them having any name.
Thanks for the illuminating examples. Agreed.
> Are there functions declared in strvec.h you have in mind that their
> parameters are confusing and hard to guess what they mean?
TBH I only learned recently (while writing the patch in this thread)
that parameter names in prototypes were optional. I got a little
confused initially when looking at strvec.h for the first time because
none of the parameters were named. Having thought a bit more about these
functions, none of them have repeated types like in your example where
naming is warranted, so I think they're fine as is.
OTOH if we were treating these .h files as something meant for direct
external consumption (that is, if strvec.h is libified and external
users outside of Git are expected to use it directly as their first
point of documentation), at that point it might make sense to name the
parameters (akin to the style of manpages for syscalls). But I imagine
at that point we would have some other means of developer docs (beyond
raw header files) for libified parts of Git, so even in that case it's
probably fine to keep things as is.
Thanks. |
|
On the Git mailing list, Jeff King wrote (reply to this): On Fri, Jan 12, 2024 at 04:37:46PM -0800, Linus Arver wrote:
> OTOH if we were treating these .h files as something meant for direct
> external consumption (that is, if strvec.h is libified and external
> users outside of Git are expected to use it directly as their first
> point of documentation), at that point it might make sense to name the
> parameters (akin to the style of manpages for syscalls). But I imagine
> at that point we would have some other means of developer docs (beyond
> raw header files) for libified parts of Git, so even in that case it's
> probably fine to keep things as is.
I think this is mostly orthogonal to libification. Whether the audience
is other parts of Git or users outside of Git, they need to know how to
call the function. Our main source of documentation there is comments
above the declaration (we've marked these with "/**" which would allow a
parser to pull them into a separate doc file, but AFAIK in the 9 years
since we started that convention, nobody has bothered to write such a
script).
Naming the parameters can help when writing those comments, because you
can then refer to them (e.g., see the comment above strbuf_addftime).
Even without that, I think they can be helpful, but I don't think I'd
bother adding them in unless taking a pass over the whole file, looking
for comments that do not sufficiently explain their matching functions.
I don't doubt that some of that would be necessary for libification,
just to increase the quality of the documentation. But I think it's
largely separate from the patch in this thread.
-Peff |
|
On the Git mailing list, Linus Arver wrote (reply to this): Jeff King <peff@peff.net> writes:
> On Fri, Jan 12, 2024 at 04:37:46PM -0800, Linus Arver wrote:
>
>> OTOH if we were treating these .h files as something meant for direct
>> external consumption (that is, if strvec.h is libified and external
>> users outside of Git are expected to use it directly as their first
>> point of documentation), at that point it might make sense to name the
>> parameters (akin to the style of manpages for syscalls). But I imagine
>> at that point we would have some other means of developer docs (beyond
>> raw header files) for libified parts of Git, so even in that case it's
>> probably fine to keep things as is.
>
> I think this is mostly orthogonal to libification. Whether the audience
> is other parts of Git or users outside of Git, they need to know how to
> call the function. Our main source of documentation there is comments
> above the declaration (we've marked these with "/**" which would allow a
> parser to pull them into a separate doc file, but AFAIK in the 9 years
> since we started that convention, nobody has bothered to write such a
> script).
>
> Naming the parameters can help when writing those comments, because you
> can then refer to them (e.g., see the comment above strbuf_addftime).
> Even without that, I think they can be helpful, but I don't think I'd
> bother adding them in unless taking a pass over the whole file, looking
> for comments that do not sufficiently explain their matching functions.
So in summary you are saying that the comments are the most important
source of documentation that we have currently, and unless naming the
parameters improves these comments, we shouldn't bother naming these
parameters. I agree.
> I don't doubt that some of that would be necessary for libification,
> just to increase the quality of the documentation. But I think it's
> largely separate from the patch in this thread.
I agree with both statements. Thanks. |
|
This patch series was integrated into seen via git@4beaaf8. |
|
There was a status update in the "New Topics" section about the branch Comment fix. Will merge to 'next'. source: <pull.1640.git.1705043195997.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@caf45d1. |
|
This patch series was integrated into seen via git@54228f5. |
|
This patch series was integrated into seen via git@ccbcffd. |
|
This patch series was integrated into seen via git@c23331d. |
|
This patch series was integrated into next via git@120ef16. |
|
There was a status update in the "Cooking" section about the branch Comment fix. Will merge to 'master'. source: <pull.1640.git.1705043195997.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@31b903d. |
|
This patch series was integrated into seen via git@c081807. |
|
This patch series was integrated into seen via git@ab28716. |
|
There was a status update in the "Cooking" section about the branch Comment fix. Will merge to 'master'. source: <pull.1640.git.1705043195997.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@dcefc33. |
|
This patch series was integrated into seen via git@46277e3. |
|
This patch series was integrated into seen via git@bc554e6. |
|
This patch series was integrated into master via git@bc554e6. |
|
This patch series was integrated into next via git@bc554e6. |
|
Closed via bc554e6. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cc: Jeff King peff@peff.net