From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 04 Dec 2025 11:09:55 GMT
Subject: Re: my complaints with clar
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:
> 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 :)

> >    # 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