Re: [PATCH 4/4] Add a function string_list_longest_prefix()
On 09/10/2012 06:33 PM, Jeff King wrote: > On Mon, Sep 10, 2012 at 09:24:17AM -0700, Junio C Hamano wrote: > >>> While we're on the subject, it seems to me that documenting APIs like >>> these in separate files under Documentation/technical rather than in the >>> header files themselves >>> >>> - makes the documentation for a particular function harder to find, >>> >>> - makes it easier for the documentation to get out of sync with the >>> actual collection of functions (e.g., the 5 undocumented functions >>> listed above). >>> >>> - makes it awkward for the documentation to refer to particular function >>> parameters by name. >>> >>> While it is nice to have a high-level prose description of an API, I am >>> often frustrated by the lack of "docstrings" in the header file where a >>> function is declared. The high-level description of an API could be put >>> at the top of the header file. >>> >>> Also, better documentation in header files could enable the automatic >>> generation of API docs (e.g., via doxygen). >> >> Yeah, perhaps you may want to look into doing an automated >> generation of Documentation/technical/api-*.txt files out of the >> headers. > > I was just documenting something in technical/api-* the other day, and > had the same feeling. I'd be very happy if we moved to some kind of > literate-programming system. I have no idea which ones are good or bad, > though. I have used doxygen, but all I remember is it being painfully > baroque. I'd much rather have something simple and lightweight, with an > easy markup format. For example, this: > >http://tomdoc.org/ > > Looks much nicer to me than most doxygen I've seen. But again, it's been > a while, so maybe doxygen is nicer than I remember. > Doxygen has a the very nifty feature of being able to generate callgraphs though. We use it extensively at $dayjob, so if you need a hand building something sensible out of git's headers, I'd be happy to help. libgit2 uses doxygen btw, and has done since the start. If we ever merge the two, it would be neat to use the same. -- Andreas Ericsson andreas.erics...@op5.se OP5 AB www.op5.se Tel: +46 8-230225 Fax: +46 8-230231 Considering the successes of the wars on alcohol, poverty, drugs and terror, I think we should give some serious thought to declaring war on peace. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] Add a function string_list_longest_prefix()
On Mon, Sep 10, 2012 at 09:24:17AM -0700, Junio C Hamano wrote: > > While we're on the subject, it seems to me that documenting APIs like > > these in separate files under Documentation/technical rather than in the > > header files themselves > > > > - makes the documentation for a particular function harder to find, > > > > - makes it easier for the documentation to get out of sync with the > > actual collection of functions (e.g., the 5 undocumented functions > > listed above). > > > > - makes it awkward for the documentation to refer to particular function > > parameters by name. > > > > While it is nice to have a high-level prose description of an API, I am > > often frustrated by the lack of "docstrings" in the header file where a > > function is declared. The high-level description of an API could be put > > at the top of the header file. > > > > Also, better documentation in header files could enable the automatic > > generation of API docs (e.g., via doxygen). > > Yeah, perhaps you may want to look into doing an automated > generation of Documentation/technical/api-*.txt files out of the > headers. I was just documenting something in technical/api-* the other day, and had the same feeling. I'd be very happy if we moved to some kind of literate-programming system. I have no idea which ones are good or bad, though. I have used doxygen, but all I remember is it being painfully baroque. I'd much rather have something simple and lightweight, with an easy markup format. For example, this: http://tomdoc.org/ Looks much nicer to me than most doxygen I've seen. But again, it's been a while, so maybe doxygen is nicer than I remember. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] Add a function string_list_longest_prefix()
Michael Haggerty writes: > Another idea: in string-list.h, one could name parameters "sorted_list" > when they must be sorted as a precondition of the function. That sounds like a very sensible thing to do. > But before getting too hung up on finery, the effort might be better > invested adding documentation for functions that are totally lacking it, > like > > string_list_clear_func() > for_each_string_list() > for_each_string_list_item() > string_list_find_insert_index() > string_list_insert_at_index() > > While we're on the subject, it seems to me that documenting APIs like > these in separate files under Documentation/technical rather than in the > header files themselves > > - makes the documentation for a particular function harder to find, > > - makes it easier for the documentation to get out of sync with the > actual collection of functions (e.g., the 5 undocumented functions > listed above). > > - makes it awkward for the documentation to refer to particular function > parameters by name. > > While it is nice to have a high-level prose description of an API, I am > often frustrated by the lack of "docstrings" in the header file where a > function is declared. The high-level description of an API could be put > at the top of the header file. > > Also, better documentation in header files could enable the automatic > generation of API docs (e.g., via doxygen). Yeah, perhaps you may want to look into doing an automated generation of Documentation/technical/api-*.txt files out of the headers. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] Add a function string_list_longest_prefix()
On 09/09/2012 11:54 AM, Junio C Hamano wrote: > Michael Haggerty writes: > >> [...] >> diff --git a/Documentation/technical/api-string-list.txt >> b/Documentation/technical/api-string-list.txt >> index 9206f8f..291ac4c 100644 >> --- a/Documentation/technical/api-string-list.txt >> +++ b/Documentation/technical/api-string-list.txt >> @@ -68,6 +68,14 @@ Functions >> to be deleted. Preserve the order of the items that are >> retained. >> >> +`string_list_longest_prefix`:: >> + >> +Return the longest string within a string_list that is a >> +prefix (in the sense of prefixcmp()) of the specified string, >> +or NULL if no such prefix exists. This function does not >> +require the string_list to be sorted (it does a linear >> +search). >> + >> `print_string_list`:: > > This may feel like outside the scope of this series, but since this > series will be the main culprit for adding many new functions to > this API in the recent history... > > - We may want to name things a bit more consistently so that people >can tell which ones can be called on any string list, which ones >are sorted list only, and which ones are unsorted one only. > >In addition, the last category _may_ need a bit more thought. >Calling unsorted_string_list_lookup() on an already sorted list >is not a crime---it is just a stupid thing to do. Yes, this could be clearer. Though I'm skeptical that a naming convention can capture all of the variation without being too cumbersome. Another idea: in string-list.h, one could name parameters "sorted_list" when they must be sorted as a precondition of the function. But before getting too hung up on finery, the effort might be better invested adding documentation for functions that are totally lacking it, like string_list_clear_func() for_each_string_list() for_each_string_list_item() string_list_find_insert_index() string_list_insert_at_index() While we're on the subject, it seems to me that documenting APIs like these in separate files under Documentation/technical rather than in the header files themselves - makes the documentation for a particular function harder to find, - makes it easier for the documentation to get out of sync with the actual collection of functions (e.g., the 5 undocumented functions listed above). - makes it awkward for the documentation to refer to particular function parameters by name. While it is nice to have a high-level prose description of an API, I am often frustrated by the lack of "docstrings" in the header file where a function is declared. The high-level description of an API could be put at the top of the header file. Also, better documentation in header files could enable the automatic generation of API docs (e.g., via doxygen). Is there some reason for the current documentation policy or is it historical and just needs somebody to put in the work to change it? > - Why are these new functions described at the top, not appended at >the bottom? I would have expected either an alphabetical, or a >more generic ones first (i.e. print and clear are a lot "easier" >ones compared to filter and prefix that are very much more >specialized). The order seemed logical to me at the time (given the constraint that functions are grouped by sorted/unsorted/don't-care): print_string_list() is only useful for debugging, so it seemed to belong below the "production" functions. string_list_clear() was already below print_string_list() (which I guessed was because it is logically used last in the life of a string_list) so I left it at the end of its section. My preference would probably have been to move print_string_list() below string_list_clear(), but somebody else made the opposite choice so I decided to respect it. That being said, I don't have anything against a different order. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] Add a function string_list_longest_prefix()
Michael Haggerty writes:
> Signed-off-by: Michael Haggerty
> ---
> Documentation/technical/api-string-list.txt | 8
> string-list.c | 20 +++
> string-list.h | 8
> t/t0063-string-list.sh | 30
> +
> test-string-list.c | 22 +
> 5 files changed, 88 insertions(+)
>
> diff --git a/Documentation/technical/api-string-list.txt
> b/Documentation/technical/api-string-list.txt
> index 9206f8f..291ac4c 100644
> --- a/Documentation/technical/api-string-list.txt
> +++ b/Documentation/technical/api-string-list.txt
> @@ -68,6 +68,14 @@ Functions
> to be deleted. Preserve the order of the items that are
> retained.
>
> +`string_list_longest_prefix`::
> +
> + Return the longest string within a string_list that is a
> + prefix (in the sense of prefixcmp()) of the specified string,
> + or NULL if no such prefix exists. This function does not
> + require the string_list to be sorted (it does a linear
> + search).
> +
> `print_string_list`::
This may feel like outside the scope of this series, but since this
series will be the main culprit for adding many new functions to
this API in the recent history...
- We may want to name things a bit more consistently so that people
can tell which ones can be called on any string list, which ones
are sorted list only, and which ones are unsorted one only.
In addition, the last category _may_ need a bit more thought.
Calling unsorted_string_list_lookup() on an already sorted list
is not a crime---it is just a stupid thing to do.
- Why are these new functions described at the top, not appended at
the bottom? I would have expected either an alphabetical, or a
more generic ones first (i.e. print and clear are a lot "easier"
ones compared to filter and prefix that are very much more
specialized).
> diff --git a/string-list.c b/string-list.c
> index bfef6cf..043f6c4 100644
> --- a/string-list.c
> +++ b/string-list.c
> @@ -136,6 +136,26 @@ void filter_string_list(struct string_list *list, int
> free_util,
> list->nr = dst;
> }
>
> +char *string_list_longest_prefix(const struct string_list *prefixes,
> + const char *string)
> +{
> + int i, max_len = -1;
> + char *retval = NULL;
> +
> + for (i = 0; i < prefixes->nr; i++) {
> + char *prefix = prefixes->items[i].string;
> + if (!prefixcmp(string, prefix)) {
> + int len = strlen(prefix);
> + if (len > max_len) {
> + retval = prefix;
> + max_len = len;
> + }
> + }
> + }
> +
> + return retval;
> +}
> +
> void string_list_clear(struct string_list *list, int free_util)
> {
> if (list->items) {
> diff --git a/string-list.h b/string-list.h
> index c4dc659..680916c 100644
> --- a/string-list.h
> +++ b/string-list.h
> @@ -38,6 +38,14 @@ int for_each_string_list(struct string_list *list,
> void filter_string_list(struct string_list *list, int free_util,
> string_list_each_func_t fn, void *cb_data);
>
> +/*
> + * Return the longest string in prefixes that is a prefix (in the
> + * sense of prefixcmp()) of string, or NULL if no such prefix exists.
> + * This function does not require the string_list to be sorted (it
> + * does a linear search).
> + */
> +char *string_list_longest_prefix(const struct string_list *prefixes, const
> char *string);
> +
>
> /* Use these functions only on sorted lists: */
> int string_list_has_string(const struct string_list *list, const char
> *string);
> diff --git a/t/t0063-string-list.sh b/t/t0063-string-list.sh
> index 0eede83..fa96eba 100755
> --- a/t/t0063-string-list.sh
> +++ b/t/t0063-string-list.sh
> @@ -15,6 +15,14 @@ string_list_split_in_place() {
> "
> }
>
> +longest_prefix() {
> + test "$(test-string-list longest_prefix "$1" "$2")" = "$3"
> +}
> +
> +no_longest_prefix() {
> + test_must_fail test-string-list longest_prefix "$1" "$2"
> +}
> +
> string_list_split_in_place "foo:bar:baz" ":" "-1" < 3
> [0]: "foo"
> @@ -60,4 +68,26 @@ string_list_split_in_place ":" ":" "-1" < [1]: ""
> EOF
>
> +test_expect_success "test longest_prefix" '
> + no_longest_prefix - '' &&
> + no_longest_prefix - x &&
> + longest_prefix "" x "" &&
> + longest_prefix x x x &&
> + longest_prefix "" foo "" &&
> + longest_prefix : foo "" &&
> + longest_prefix f foo f &&
> + longest_prefix foo foobar foo &&
> + longest_prefix foo foo foo &&
> + no_longest_prefix bar foo &&
> + no_longest_prefix bar:bar foo &&
> + no_longest_prefix foobar foo &&
> + longest_prefix foo:bar foo foo &&
> + longest_prefix foo:bar bar bar &&
> + longest_prefix foo::bar foo foo &&
>
