Re: my complaints with clar
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Dec 4, 2025, 11:09 UTC
- Message-ID
- <aTFsA-jJqcRZJs53@pks.im>
- In-Reply-To
- <bd0a8a76-fccb-4b6c-abb7-b53dd890e9e0@gmail.com>
On Mon, Dec 01, 2025 at 02:16:13PM +0000, Phillip Wood wrote:
Show 27 quoted lines
> On 30/11/2025 13:46, Jeff King wrote: > > > + /* > > > + * Do not use cl_assert_equal_i_fmt(..., PRIuMAX) here. The macro > > > + * casts to int under the hood, corrupting the values. > > > + */ > > > + clar__assert_equal(CLAR_CURRENT_FILE, CLAR_CURRENT_FUNC, > > > + CLAR_CURRENT_LINE, > > > + "expect_result != result", 1, > > > + "%"PRIuMAX, expect_result, result); > > > +} > > > > This was an exciting bug to track down. If you use i_fmt() here, you get > > some neat undefined behavior. It worked for gcc, but failed with clang > > (but only with -O2!). > > > > Obviously this was me using it wrong, and the "i" in the macro should > > have been a hint. But this invocation is kind of ugly, with the explicit > > mentions of internal CLAR variables. clar__assert_equal() understands > > PRIuMAX as a comparator, but there doesn't appear to be any macro to use > > it nicely. > > > > Should there be a generic cl_assert_equal() that fills in the first > > few parameters but is otherwise type-agnostic? > > Patrick's got a PR open for that at > https://github.com/clar-test/clar/pull/117 it seems to have got stuck > because of a lack of review.
Yeah, this is indeed a long-standing issue. As Phillip mentioned I've already had the fix pending, but I lost track and just never merged it. Phillip now left a review, and I've polished the PR a bit. I'll wait a few more days before merging it, and then these issues will be a thing of the past :)
Show 50 quoted lines
> > # start of suite 10: parse_int
> > not ok 59 - parse_int::basic
> > ---
> > reason: |
> > expect_result != result
> > 10 != 11
> > at:
> > file: 't/unit-tests/u-parse-int.c'
> > line: 41
> > function: 'test_parse_int__basic'
> > ---
> >
> > OK, but "prove t/unit-tests/bin/unit-tests" gives me:
> >
> > t/unit-tests/bin/unit-tests .. Failed 1/59 subtests
> > Test Summary Report
> > -------------------
> > t/unit-tests/bin/unit-tests (Wstat: (none) Tests: 59 Failed: 1)
> > Failed test: 59
> > Parse errors: Badly formed hash line: '---' at /usr/share/perl/5.40/TAP/Parser/YAMLish/Reader.pm line 244.
> >
> > Yuck. It actually does have what I need (that test 59 was the failure),
> > so the extra parse error is mostly a red herring (though it does prevent
> > us finding any further failures). I think in TAP that arbitrary output
> > is supposed to be prefixed with a "#".
>
> TAP also allows you to embed YAML and unfortunately that's what clar tries
> to do but that last " ---" line should be " ...". With the diff below
> (which I'm afraid thunderbird will probably mangle) prove parses the output
> correctly but still does not print the error message. I'll update clar's
> self tests and open a PR later this week.
>
> ---- 8< ----
> diff --git a/t/unit-tests/clar/clar/print.h b/t/unit-tests/clar/clar/print.h
> index 89b66591d75..6a2321b399d 100644
> --- a/t/unit-tests/clar/clar/print.h
> +++ b/t/unit-tests/clar/clar/print.h
> @@ -164,7 +164,7 @@ static void clar_print_tap_ontest(const char
> *suite_name, const char *test_name,
> printf(" file: '");
> print_escaped(error->file); printf("'\n");
> printf(" line: %" PRIuMAX "\n",
> error->line_number);
> printf(" function: '%s'\n", error->function);
> - printf(" ---\n");
> + printf(" ...\n");
> }
>
> break;
> ---- >8 ----Indeed, this was a plain bug. I've merged your upstream PR, thanks!
I'll send a pull request soonish to update our own version of clar.
Thanks for the feedback! Hope that the pending changes will improve the status quo.
Patrick