Hi,
Cool that you are working on this! See my comments below.
On 05/30/2016 12:36 PM, William Duclot wrote:
> Test the strbuf API. Being used throughout all Git the API could be
> considered tested, but adding specific tests makes it easier to improve
> and extend the API.
> ---
> Makefile | 1 +
> t/helper/test-strbuf.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++++
> t/t0082-strbuf.sh | 19 ++++++++++++++
> 3 files changed, 89 insertions(+)
> create mode 100644 t/helper/test-strbuf.c
> create mode 100755 t/t0082-strbuf.sh
>
> diff --git a/Makefile b/Makefile
> index 3f03366..dc84f43 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -613,6 +613,7 @@ TEST_PROGRAMS_NEED_X += test-scrap-cache-tree
> TEST_PROGRAMS_NEED_X += test-sha1
> TEST_PROGRAMS_NEED_X += test-sha1-array
> TEST_PROGRAMS_NEED_X += test-sigchain
> +TEST_PROGRAMS_NEED_X += test-strbuf
> TEST_PROGRAMS_NEED_X += test-string-list
> TEST_PROGRAMS_NEED_X += test-submodule-config
> TEST_PROGRAMS_NEED_X += test-subprocess
> diff --git a/t/helper/test-strbuf.c b/t/helper/test-strbuf.c
> new file mode 100644
> index 0000000..622f627
> --- /dev/null
> +++ b/t/helper/test-strbuf.c
> @@ -0,0 +1,69 @@
> +#include "git-compat-util.h"
> +#include "strbuf.h"
> +
> +/*
> + * Check behavior on usual use cases
> + */
> +int test_usual(struct strbuf *sb)
Is there a particular reason that you pass `sb` into the function? Why
not use a local variable?
It wouldn't hurt to declare this function static, because it is only
used within this one compilation unit. On the other hand, in this
particular case I don't think it matters much one way or the other.
> +{
> + size_t size, old_alloc;
> + char *res, *old_buf, *str_test = malloc(5*sizeof(char));
There is no reason to multiply by `sizeof(char)` here, and we don't do
it in our code. (According to the C standard, `sizeof(char)` is always 1.)
> + strbuf_grow(sb, 1);
> + strcpy(str_test, "test");
> + old_alloc = sb->alloc;
> + strbuf_grow(sb, 1000);
> + if (old_alloc == sb->alloc)
> + die("strbuf_grow does not realloc the buffer as expected");
It's not ideal that your test depends on the details of the allocation
policy of strbuf. If somebody were to decide that it makes sense for the
initial allocation of a strbuf to be 1024 characters, this test would
break. I know it's implausible, but it's better to remove this coupling.
You could do it by ensuring that you request more space than is already
allocated:
strbuf_grow(sb, sb.alloc - sb.len + 1000);
On the other hand, if you *want* to test the details of strbuf's
allocation policy here, you would do it explicitly rather than as the
side effect of this test. For example, before calling `strbuf_grow(sb,
1000)`, you could insert:
if (sb.alloc > SOME_VALUE)
die("strbuf_grow(sb, 1) allocated too much space");
Though my opinion is that if you want to write tests of strbuf's
allocation policies, it should be in an entirely separate test.
> + old_buf = sb->buf;
> + res = strbuf_detach(sb, &size);
Since you don't use `size`, you can pass `NULL` as the second argument
of `strbuf_detach()`. The same below.
Alternatively, maybe you want to add a test that `strbuf_detach()` sets
`size` to the expected value.
> + if (res != old_buf)
> + die("strbuf_detach does not return the expected buffer");
> + free(res);
> +
> + strcpy(str_test, "test");
Near the beginning of the function you have this exact line, but here
you do it again even though str_test wasn't touched between the two
lines. One of them can be deleted...
...but in fact it would be easier to initialize str_test using our
xstrdup() function:
char *str_test = xstrdup("test");
> + strbuf_attach(sb, (void *)str_test, strlen(str_test), sizeof(str_test));
Since str_test is a `char *`, `sizeof(str_test)` returns the size of the
pointer itself (e.g., 4 or 8 bytes, depending on your computer's
architecture). What you want here is the size of the memory that it
points at, which in this case is `strlen(str_test) + 1`.
(You may be confusing this pattern with code that looks like this:
char str_test[5];
In this case, `sizeof(str_test)` is indeed 5, because `str_test` is an
array of characters rather than a pointer to a character. It's confusing.)
Also, you don't need to cast `str_test` to `void *`. Any pointer can be
converted to `void *` implicitly.
> + res = strbuf_detach(sb, &size);
> + if (res != str_test)
> + die("strbuf_detach does not return the expected buffer");
> + free(res);
> + strbuf_release(sb);
> +
> + return 0;
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + size_t size = 1;
> + struct strbuf sb;
> + char str_test[5] = "test";
> + char str_foo[7] = "foo";
`size`, `str_test`, and `str_foo` are all unused.
You should compile using `DEVELOPER=1` to enable a bunch of compiler
warnings that Git developers tend to use. Any code you write should
compile without warnings or errors when compiled with that setting. See
`Documentation/CodingGuidelines` for more info.
> +
> + if (argc != 2)
> + usage("test-strbuf mode");
> +
> + if (!strcmp(argv[1], "basic_grow")) {
> + /*
> + * Check if strbuf_grow(0) allocate a new NUL-terminated buffer
> + */
> + strbuf_init(&sb, 0);
> + strbuf_grow(&sb, 0);
> + if (sb.buf == strbuf_slopbuf)
> + die("strbuf_grow failed to alloc memory");
> + strbuf_release(&sb);
> + if (sb.buf != strbuf_slopbuf)
> + die("strbuf_release does not reinitialize the strbuf");
> + } else if (!strcmp(argv[1], "strbuf_check_behavior")) {
> + strbuf_init(&sb, 0);
> + return test_usual(&sb);
> + } else if (!strcmp(argv[1], "grow_overflow")) {
> + /*
> + * size_t overflow: should die()
> + */
> + strbuf_init(&sb, 1000);
> + strbuf_grow(&sb, maximum_unsigned_value_of_type((size_t)1));
> + } else {
> + usage("test-strbuf mode");
> + }
Consider putting each test in a separate function, rather than
implementing some tests inline and one as a function. Consistency makes
code easier to read.
> [...]
Michael
--
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