Re: [RFC PATCH 3/5] perf test: Make each test/suite its own struct.

From: Ian Rogers <irogers@google.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Jin Yao <yao.jin@linux.intel.com>,
	John Garry <john.garry@huawei.com>,
	"Paul A . Clarke" <pc@us.ibm.com>,
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
	Brendan Higgins <brendanhiggins@google.com>,
	Daniel Latypov <dlatypov@google.com>,
	David Gow <davidgow@google.com>,
	eranian@google.com
Subject: Re: [RFC PATCH 3/5] perf test: Make each test/suite its own struct.
Date: Mon, 27 Sep 2021 09:08:53 -0700	[thread overview]
Message-ID: <CAP-5=fWONeXLa=r4uJ1TM4K+E1moBMrV29LzfbciTfVVHvorsA@mail.gmail.com> (raw)
In-Reply-To: <YVDiMDoD2dr312KY@krava>

On Sun, Sep 26, 2021 at 2:12 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, Sep 22, 2021 at 01:19:56AM -0700, Ian Rogers wrote:
> > By switching to an array of pointers to tests (later to be suites)
> > the definition of the tests can be moved to the file containing the
> > tests.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/arch/arm/include/arch-tests.h     |   2 +-
> >  tools/perf/arch/arm/tests/arch-tests.c       |  21 +-
> >  tools/perf/arch/arm64/include/arch-tests.h   |   2 +-
> >  tools/perf/arch/arm64/tests/arch-tests.c     |  15 +-
> >  tools/perf/arch/powerpc/include/arch-tests.h |   2 +-
> >  tools/perf/arch/powerpc/tests/arch-tests.c   |  15 +-
> >  tools/perf/arch/x86/include/arch-tests.h     |   2 +-
> >  tools/perf/arch/x86/tests/arch-tests.c       |  47 ++--
> >  tools/perf/tests/builtin-test.c              | 273 ++++++++++++-------
> >  tools/perf/tests/tests.h                     |   6 +
> >  10 files changed, 220 insertions(+), 165 deletions(-)
> >
> > diff --git a/tools/perf/arch/arm/include/arch-tests.h b/tools/perf/arch/arm/include/arch-tests.h
> > index c62538052404..37039e80f18b 100644
> > --- a/tools/perf/arch/arm/include/arch-tests.h
> > +++ b/tools/perf/arch/arm/include/arch-tests.h
> > @@ -2,6 +2,6 @@
> >  #ifndef ARCH_TESTS_H
> >  #define ARCH_TESTS_H
> >
> > -extern struct test arch_tests[];
> > +extern struct test *arch_tests[];
> >
> >  #endif
> > diff --git a/tools/perf/arch/arm/tests/arch-tests.c b/tools/perf/arch/arm/tests/arch-tests.c
> > index 6848101a855f..4374b0293177 100644
> > --- a/tools/perf/arch/arm/tests/arch-tests.c
> > +++ b/tools/perf/arch/arm/tests/arch-tests.c
> > @@ -3,18 +3,15 @@
> >  #include "tests/tests.h"
> >  #include "arch-tests.h"
> >
> > -struct test arch_tests[] = {
> >  #ifdef HAVE_DWARF_UNWIND_SUPPORT
> > -     {
> > -             .desc = "DWARF unwind",
> > -             .func = test__dwarf_unwind,
> > -     },
> > +DEFINE_SUITE("DWARF unwind", dwarf_unwind);
>
> why not having this and other in here DEFINE_SUITE in
> tests/dwarf-unwind.c ? it seems to get compiled in only
> for supported arch

Not sure I follow. In these changes I've just tried to move things
around without changing the logic behind things. This sounds like a
sensible follow up clean up.

Thanks,
Ian

> jirka
>
> >  #endif
> > -     {
> > -             .desc = "Vectors page",
> > -             .func = test__vectors_page,
> > -     },
> > -     {
> > -             .func = NULL,
> > -     },
> > +DEFINE_SUITE("Vectors page", vectors_page);
> > +
> > +struct test *arch_tests[] = {
> > +#ifdef HAVE_DWARF_UNWIND_SUPPORT
> > +     &dwarf_unwind,
> > +#endif
> > +     &vectors_pages,
> > +     NULL,
> >  };
>
> SNIP
>