Hi René
On 05/07/2024 19:01, René Scharfe wrote:
Am 05.07.24 um 11:42 schrieb phillip.wood123@xxxxxxxxx:
On 02/07/2024 21:55, René Scharfe wrote:
Am 02.07.24 um 17:13 schrieb phillip.wood123@xxxxxxxxx:
The changes in patch 6 to use TEST_RUN() mean that each test now has
more boilerplate to initialize and free the strbuf.
This makes them more similar to strbuf usage in the wild. Using
the API idiomatically just makes more sense to me.
I see what you mean. I think it only looks idiomatic if you're
already familiar with the api though as the test bodies call wrappers
rather than using the strbuf api directly. I think that reduces its
value as an example of idomatic usage for someone who is not familiar
with the strbuf api.
In early versions I used the original names by adding these just before
main():
#define strbuf_addch(x, y) t_addch(x, y)
#define strbuf_addstr(x, y) t_addstr(x, y)
#define strbuf_release(x) t_release(x)
This allowed normal looking code to be used in tests, with checks
injected behind the scenes. Rejected it for v1 because it offers no
structural improvements, just optics. It does allow to forget the
checked versions when writing tests, though, so perhaps it's still
worth doing.
It also obscures what the test is doing though. It's nice if unit tests
show how to use an API but I don't think we should let that be our
overriding concern.
We could also let xstrvfmt() call vsnprintf(3) directly. The code
duplication would be a bit grating, but perhaps there's some good
base function hidden in there somewhere.
Oh, interesting - maybe something like
char* xstrvfmt(const char *fmt, ...)
{
va_list ap, aq;
va_start(ap, fmt);
va_copy(aq, ap);
len = vnsprintf(NULL, 0, fmt, ap);
if (len < 0)
BUG(...)
buf = xmalloc(len + 1);
if (vnsprintf(buf, len + 1, fmt, aq) != len)
BUG(...)
va_end(aq);
va_end(ap);
return buf;
}
Yes. Though the current version allocates 65 bytes in the first try
and only needs to call vnsprintf(3) once if the output fits in. No
longer doing that might affect the performance of some callers in a
noticeable way.
Good point
I'm not very enthusiastic about requiring the test author to wrap
TEST_RUN() in an if() statement rather than just doing that for them.
It makes it explicit but from the test author's point of view it just
feels like pointless boilerplate.
Hmm. We can add more magic, but I suspect that it might confuse
developers and editors.
To me its confusing to have to wrap TEST_RUN() in an if() statement
until one realizes that the test might be skipped. If we document
that the test body should be enclosed in braces I don't think it
should confuse developers or editors and will keep the tests a bit
cleaner.
You don't need braces in either case. I.e. something like
if (TEST_RUN("foo"))
foo();
works fine. And
#define IF_TEST_RUN(...) if (TEST_RUN(__VA_ARGS__))
IF_TEST_RUN("foo")
foo();
works fine as well.
Indeed. The reason I suggested requiring braces was to help editors
indent the code correctly and because it gives a nice visual cue to see
what is in each test.
Best Wishes
Phillip
The confusion that I'm afraid of is that we'd effectively invent a new
control flow keyword here, which is unusual. There are precedents,
though: foreach macros like for_each_string_list_item. We tell
clang-format about them using its option ForEachMacros. I see it also
has an option IfMacros since version 13 (unused by us so far). Hmm.
René
