Re: my complaints with clar
From: Patrick Steinhardt <hidden>
Date: 2025-12-04 11:10:02
On Mon, Dec 01, 2025 at 02:16:13PM +0000, Phillip Wood wrote:
On 30/11/2025 13:46, Jeff King wrote:quoted
quoted
+ /* + * 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 :)
quoted hunk ↗ jump to hunk
quoted
# 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