Hi Thiago,
Thiago Farina wrote:
> Add a simple test that demonstrates how to create and manipulate
> a list of strings using the string-list.h API.
Quick thoughts:
> --- /dev/null
> +++ b/test-string-list.c
> @@ -0,0 +1,29 @@
> +#include <stdio.h>
Git programs tend to start with
#include "cache.h"
or
#include "git-compat-util.h"
to get all the portability niceties.
> + print_string_list(&list, "");
> +
> + int has_foo = string_list_has_string(&list, "foo");
Whitespace, declaration after statement... (see
Documentation/CodingGuidelines).
> + if (has_foo != 1)
> + error("List doesn't have foo.");
This does not exit with nonzero status when it fails. You probably
wanted
if (bad things)
return error("problems!");
> + string_list_clear(&list, 0);
> + if (list.nr > 0)
> + error("List is not clear.");
To make sure this example remains valid, wouldn't you want to include
a caller in the t/ directory so it can be automatically run? (See
t/README.)
Thoughts separate from the code:
* it is probably worth mentioning Documentation/technical/api-string-list.txt
for people who do not know about it.
* for this to be useful as a test I think one has to sort of believe
that it can break. That is, a test of something this basic (which
is already demonstrated and exercised by code throughout git, after
all) would tend to be especially devious.
* api-string-list.txt does not mention the STRING_LIST_INIT macros
you introduced. Maybe that would be worth improving.
Regards,
Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html