git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "René Scharfe" <l.s.r@web.de>
To: phillip.wood@dunelm.org.uk, git@vger.kernel.org
Cc: Josh Steadmon <steadmon@google.com>,
	Achu Luma <ach.lumap@gmail.com>,
	Christian Couder <christian.couder@gmail.com>
Subject: Re: [PATCH v2 4/4] t-ctype: avoid duplicating class names
Date: Wed, 6 Mar 2024 19:16:37 +0100	[thread overview]
Message-ID: <3ef0927f-4d7b-4061-925e-c113d1c8730d@web.de> (raw)
In-Reply-To: <0947cb09-8b07-4fcd-bbe2-ae37c2cd5ec7@gmail.com>

Hello Phillip,

Am 04.03.24 um 10:51 schrieb Phillip Wood:
> On 03/03/2024 10:13, René Scharfe wrote:
>> TEST_CTYPE_FUNC defines a function for testing a character classifier,
>> TEST_CHAR_CLASS calls it, causing the class name to be mentioned twice.
>>
>> Avoid the need to define a class-specific function by letting
>> TEST_CHAR_CLASS do all the work.  This is done by using the internal
>> functions test__run_begin() and test__run_end(), but they do exist to be
>> used in test macros after all.
>
> Those internal functions exist to implement the TEST() macro, they
> are not really intended for use outside that (which is why they are
> marked as private in the header file). If we ever want to update the
> implementation of TEST() it will be a lot harder if we're using the
> internal implementation directly in test files. Unit tests should be
> wrapping TEST() if it is appropriate but not the internal
> implementation directly.

forcing tests to be expressions and not allow them to use statements is
an unusual requirement.  I don't see how the added friction would make
tests any better.  It just requires more boilerplate code and annoying
repetition.  What kind of changes do you envision that would be
hindered by allowing statements?

> Ideally we wouldn't need TEST_CTYPE_FUNC as there would only be a
> single function that was passed a ctype predicate, an input array and
> an array of expected results. Unfortunately I don't think that is
> possible due the the way the ctype predicates are implemented. Having
> separate macros to define the test function and to run the test is
> annoying but I don't think it is really worth exposing the internal
> implementation just to avoid it.

The classifiers are currently implemented as macros.  We could turn them
into inline functions and would then be able to pass them to a test
function.  Improving testability is a good idea, but also somehow feels
like the tail wagging the dog.  It would be easy, though, I think.  And
less gross than:

>> Alternatively we could unroll the loop to provide a very long expression
>> that tests all 256 characters and EOF and hand that to TEST, but that
>> seems awkward and hard to read.

... which would yield unsightly test macros and huge test binaries.  But
it would certainly be possible, and keep the definitions of the actual
tests clean.

René

  reply	other threads:[~2024-03-06 18:17 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-25 11:27 [PATCH 0/3] t-ctype: simplify unit test definitions René Scharfe
2024-02-25 11:27 ` [PATCH 1/3] t-ctype: allow NUL anywhere in the specification string René Scharfe
2024-02-25 18:05   ` Eric Sunshine
2024-02-25 18:28     ` René Scharfe
2024-02-25 18:41       ` Eric Sunshine
2024-02-25 21:00         ` Jeff King
2024-02-25 21:02           ` Eric Sunshine
2024-02-25 11:27 ` [PATCH 2/3] t-ctype: avoid duplicating class names René Scharfe
2024-02-25 11:27 ` [PATCH 3/3] t-ctype: do one test per class and char René Scharfe
2024-02-26  9:28   ` Christian Couder
2024-02-26 17:26     ` René Scharfe
2024-02-26 17:44       ` Junio C Hamano
2024-02-26 18:58       ` Josh Steadmon
2024-02-27 10:04         ` Christian Couder
2024-03-02 22:00           ` René Scharfe
2024-03-04 10:00             ` Christian Couder
2024-03-06 18:16               ` René Scharfe
2024-03-04 18:35             ` Josh Steadmon
2024-03-04 18:46               ` Junio C Hamano
2024-03-03 10:13 ` [PATCH v2 0/4] t-ctype: simplify unit test definitions René Scharfe
2024-03-03 10:13   ` [PATCH v2 1/4] t-ctype: allow NUL anywhere in the specification string René Scharfe
2024-03-03 10:13   ` [PATCH v2 2/4] t-ctype: simplify EOF check René Scharfe
2024-03-03 10:13   ` [PATCH v2 3/4] t-ctype: align output of i René Scharfe
2024-03-03 10:13   ` [PATCH v2 4/4] t-ctype: avoid duplicating class names René Scharfe
2024-03-04  9:51     ` Phillip Wood
2024-03-06 18:16       ` René Scharfe [this message]
2024-03-08 14:05         ` Phillip Wood
2024-03-09 11:28         ` Phillip Wood
2024-03-10 12:48           ` René Scharfe
2024-03-04  9:25   ` [PATCH v2 0/4] t-ctype: simplify unit test definitions Christian Couder

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3ef0927f-4d7b-4061-925e-c113d1c8730d@web.de \
    --to=l.s.r@web.de \
    --cc=ach.lumap@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=phillip.wood@dunelm.org.uk \
    --cc=steadmon@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).