Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 31 Jul 2023 10:23:33 +0200
From:   Thomas =?utf-8?Q?Wei=C3=9Fschuh?= <linux@weissschuh.net>
To:     Zhangjin Wu <falcon@tinylab.org>
Cc:     linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
        shuah@kernel.org, tanyuan@tinylab.org, w@1wt.eu
Subject: Re: [PATCH 1/4] selftests/nolibc: drop unused test helpers
Message-ID: <0d441f70-bdc3-4aa1-b392-6ec170c9a5bb@t-8ch.de>
References: <7d8dee88-16af-464f-b7f3-55b77dbc3c46@t-8ch.de>
 <20230731073243.21265-1-falcon@tinylab.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230731073243.21265-1-falcon@tinylab.org>
Precedence: bulk

On 2023-07-31 15:32:43+0800, Zhangjin Wu wrote:
> Hi, Thomas
> 
> > Note:
> > 
> > It seems your mail client does not add the prefix "Re: " to responses.
> > Is that intentional?
> >
> 
> I use a lightweight 'b4 + git send-email' method to reply emails,
> sometimes, I forgot manually adding the 'Re: ' prefix, perhaps I should
> write a simple script to do that or carefully check the Subject title
> everytime, Thanks!

Now there are two "Re: " prefixes :-)

My understanding is that there is exactly one "Re: " prefix iff the
message is any response at all.

> > On 2023-07-31 14:48:26+0800, Zhangjin Wu wrote:
> > > Hi, Thomas
> > > 
> > > > As we want to enable compiler warnings in the future these would be
> > > > reported as unused functions.
> > > > 
> > > > If we need them in the future they are easy to recreate from their still
> > > > existing siblings.
> > > > 
> > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > > ---
> > > >  tools/testing/selftests/nolibc/nolibc-test.c | 99 ----------------------------
> > > >  1 file changed, 99 deletions(-)
> > > > 
> > > > diff --git a/tools/testing/selftests/nolibc/nolibc-test.c b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > index 03b1d30f5507..53e2d448eded 100644
> > > > --- a/tools/testing/selftests/nolibc/nolibc-test.c
> > > > +++ b/tools/testing/selftests/nolibc/nolibc-test.c
> > > > @@ -161,31 +161,6 @@ static void result(int llen, enum RESULT r)
> > > >   * of failures, thus either 0 or 1.
> > > >   */
> > > >  
> > > > -#define EXPECT_ZR(cond, expr)				\
> > > > -	do { if (!(cond)) result(llen, SKIPPED); else ret += expect_zr(expr, llen); } while (0)
> > > > -
> > > > -static int expect_zr(int expr, int llen)
> > > > -{
> > > 
> > > Why not a simple 'static __attribute__((unused))' line, then, no need to
> > > add them again next time.
> > > 
> > >     -static int expect_zr(int expr, int llen)
> > >     +static __attribute__((unused))
> > >     +int expect_zr(int expr, int llen)
> > >      {
> > 
> > Personally I don't like having dead code lying around that needs to be
> > maintained and skipped over while reading.
> > It's not a given that we will need those helpers in the future at all.
> >
> 
> It is reasonable in some degree from current status, especially for
> these ones are newly added, but let us think about these scenes: when we
> would drop or change some test cases in the future and the helpers may
> would be not referenced by any test cases in a short time, and warnings
> there, but some other cases may be added later to use them again ...

That doesn't seem very likely.
Did it happen recently?

> I'm ok to drop these ones, but another patch may be required to add
> 'static __attribute__((unused))' for all of the currently used ones,
> otherwise, there will be warnings randomly by a test case change or
> drop.

Then we just drop the helper when we don't need it anymore.

I also dislike the __attribute__ spam to be honest.

> Or even further, is it possible to merge some of them to some more
> generic helpers like the ones used from the selftest.h in your last RFC
> patchset?

Something like this will indeed be part of the KTAP rework.
But it's a change for another time.

Thomas