On Wed, 8 Jan 2025 at 07:14, Patrick Steinhardt <ps@xxxxxx> wrote:
>
> On Tue, Jan 07, 2025 at 10:16:33AM -0800, Junio C Hamano wrote:
> > Seyi Kuforiji <kuforiji98@xxxxxxxxx> writes:
> >
> > > 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.
>
> Yeah. This was something I proposed, but I already mentioned to Seyi
> that I'm not all that happy with the outcome as it has a couple of
> downsides, for example broken syntax highlighting in lots of editors. I
> said he can send that version to the mailing list anyway and get some
> feedback on it to figure out whether my discomfort with my own idea is
> warranted or not. And your comment here basically confirms that my idea
> wasn't that great after all :)
>
> So I agree with you, let's scrap the idea and have proper function
> bodies instead.
>
> Patrick
Thank you for the feedback and insights. I'll update the patch to use
standard function bodies.
Thanks
Seyi
