Re: [PATCH 1/2] t/unit-tests: match functions signature with trailing code

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 7, 2025, 18:16 UTC
Message-ID: <xmqq7c76trpa.fsf@gitster.g>
In-Reply-To: <20250107091932.126673-2-kuforiji98@gmail.com>

Seyi Kuforiji <kuforiji98@gmail.com> writes:

Show 14 quoted lines

> The `generate-clar-decls.sh` script extracts signatures of test
> functions from our unit tests, which will later get used by the clar to
> automatically wire up tests. The sed command only matches lines that
> ended immediately after `void)`, causing it to miss declarations with
> additional content such as comments or annotations.
>
> Relax the regular expression by making it match lines with trailing data
> after the function signature. This ensures that all valid function
> declarations are captured and formatted as `extern` declarations
> regardless of their formatting style, improving the robustness of the
> script when parsing `$suite` files.
>
> This will be used in subsequent commits to match and capture the
> function signature correctly, regardless of any trailing content.

I am not sure if this is going in the right direction, though.

Especially for things like test suites that are looked at and worked on only by develoeprs *and* these tools, being uniform and consistent weighs more than being more flexible.

Let me state it in another way. How many of the existing test pieces are picked up by the current pattern, and among them how many of them would see vast improvements if they are allowed to have arbitrary garbage after their "I do not take any arguments" function signature? Are new tests you are migrating from outside the clar world lose a lot if they are no longer allowed to have comments there, or would it be suffice to have the comments before the functions (which many of our function definition do anyway)?

A quick peek at [PATCH 2/2] tells me that this is not even something that would make it easier to port the existing tests by allowing more straight line-by-line copies or something. The patch splits many in-line test pieces in the "main" into separate functions, and it does so in a rather unusual format, e.g.,

  void test_hash__multi_character(void) TEST_HASH_STR("abc",
          "a9993e364706816aba3e25717850c26c9cd0d89d",
          "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad")

where TEST_HASH_STR() expands to the function body that starts with a "{" and ends with a "}". It can well be written more like

    void test_hash__multi_character(void)
    {
	TEST_HASH_STR("abc",
        	"a9993e364706816aba3e25717850c26c9cd0d89d",
		"ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad");
    }

and we do not need this step at all if we did so. Such a construct would be a lot friendlier to the editors that auto-indent, too.

So, I do not quite see much value in this particular change.

Show 20 quoted lines

> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Seyi Kuforiji <kuforiji98@gmail.com>
> ---
>  t/unit-tests/generate-clar-decls.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/unit-tests/generate-clar-decls.sh b/t/unit-tests/generate-clar-decls.sh
> index 3b315c64b3..02e45cf0ba 100755
> --- a/t/unit-tests/generate-clar-decls.sh
> +++ b/t/unit-tests/generate-clar-decls.sh
> @@ -14,6 +14,6 @@ do
>  	suite_name=$(basename "$suite")
>  	suite_name=${suite_name%.c}
>  	suite_name=${suite_name#u-}
> -	sed -ne "s/^\(void test_${suite_name}__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)\)$/extern \1;/p" "$suite" ||
> +	sed -ne "s/^\(void test_${suite_name}__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)\).*/extern \1;/p" "$suite" ||
>  	exit 1
>  done >"$OUTPUT"
> --
> 2.34.1