Re: [PATCH 4/4] Add a function string_list_longest_prefix()

[Michael Haggerty <mhagger@alum.mit.edu>]

On 09/09/2012 11:54 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> 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
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/