From: Jonathan Nieder <jrnieder@gmail.com>
To: Thiago Farina <tfransosi@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [Patch v3] string-list: Document STRING_LIST_INIT_* macros.
Date: Sun, 5 Sep 2010 19:22:49 -0500 [thread overview]
Message-ID: <20100906002249.GB18060@burratino> (raw)
In-Reply-To: <1283731991-15080-1-git-send-email-tfransosi@gmail.com>
Thiago Farina wrote:
> +++ b/Documentation/technical/api-string-list.txt
> @@ -11,10 +11,15 @@ The caller:
>
> . Allocates and clears a `struct string_list` variable.
>
> -. Initializes the members. You might want to set the flag `strdup_strings`
> - if the strings should be strdup()ed. For example, this is necessary
> - when you add something like git_path("..."), since that function returns
> - a static buffer that will change with the next call to git_path().
> +. Initializes the members. A string_list might be initialize by
s/initialize/initalized/, I think.
> + `= STRING_LIST_INT_NODUP` or `= STRING_LIST_INIT_DUP` before it can be used.
> +
> + Strings in lists initialized with the _DUP variant will be
> + automatically strdup()ed on insertion and free()ed on removal.
> + For example, this is necessary when you add something like
> + git_path("..."), since that function returns a static buffer
> + that will change with the next call to git_path().
If we do not have a string_list_init() function, maybe it is worth
mentioning how a person can use
memset(&list, 0, sizeof(struct string_list));
list.strdup_strings = 1 or 0;
too?
The previous text tried (too subtly, perhaps) to imply that with the
"clears" (for memset) and "might want to set the flag `strdup_strings`"
phrases.
> @@ -34,10 +39,9 @@ member (you need this if you add things later) and you should set the
> Example:
>
> ----
> -struct string_list list;
> +struct string_list list = STRING_LIST_DUP;
> int i;
>
> -memset(&list, 0, sizeof(struct string_list));
> string_list_append(&list, "foo");
> string_list_append(&list, "bar");
> for (i = 0; i < list.nr; i++)
Probably worth copying and pasting this code to another file and
trying it to make sure it works for the final draft.
Also, I am afraid I will not be able to send detailed reviews on
uncomplicated patches like this one in the future. It simply does not
scale (though I like for people to learn, at a certain point it
becomes faster to do things myself).
So if you can, it is best to find ways to motivate other people to
help with your work, by conveying what problems it will solve (in this
case: people might be confused about the initialization sequence; as
part of patch 3 demonstrated, the invariants regarding strdup_strings
are not being described clearly enough) and finding ways to minimize
the time other people have to spend to get it done.
Regards,
Jonathan
next prev parent reply other threads:[~2010-09-06 0:24 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-09-06 0:13 [Patch v3] string-list: Document STRING_LIST_INIT_* macros Thiago Farina
2010-09-06 0:22 ` Jonathan Nieder [this message]
2010-09-06 0:38 ` Thiago Farina
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20100906002249.GB18060@burratino \
--to=jrnieder@gmail.com \
--cc=git@vger.kernel.org \
--cc=tfransosi@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).