Re: [PATCH v2 06/22] perf test: Add helper functions for abstraction.

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: [PATCH v2 06/22] perf test: Add helper functions for abstraction.
Date: Mon, 25 Oct 2021 15:35:20 -0700	[thread overview]
Message-ID: <CAP-5=fWJyUi7M9rQyafYKNeCdUKru33sO3Ld5c8RxDT60+FBTw@mail.gmail.com> (raw)
In-Reply-To: <YXAM31HKzS4/qJw/@krava>

On Wed, Oct 20, 2021 at 5:34 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, Oct 13, 2021 at 10:45:48AM -0700, Ian Rogers wrote:
>
> SNIP
>
> >       else
> >               pr_debug("%s subtest %d:", t->desc, subtest + 1);
> > @@ -218,11 +257,10 @@ static int test_and_print(struct test_suite *t, bool force_skip, int subtest)
> >               pr_info(" Ok\n");
> >               break;
> >       case TEST_SKIP: {
> > -             const char *skip_reason = NULL;
> > -             if (t->subtest.skip_reason)
> > -                     skip_reason = t->subtest.skip_reason(subtest);
> > -             if (skip_reason)
> > -                     color_fprintf(stderr, PERF_COLOR_YELLOW, " Skip (%s)\n", skip_reason);
> > +             const char *reason = skip_reason(t, subtest);
> > +
> > +             if (reason)
> > +                     color_fprintf(stderr, PERF_COLOR_YELLOW, " Skip (%s)\n", reason);
> >               else
> >                       color_fprintf(stderr, PERF_COLOR_YELLOW, " Skip\n");
> >       }
> > @@ -397,7 +435,7 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
> >       int width = shell_tests__max_desc_width();
> >
> >       for_each_test(j, k, t) {
> > -             int len = strlen(t->desc);
> > +             int len = strlen(test_description(t, -1));
> >
> >               if (width < len)
> >                       width = len;
> > @@ -407,17 +445,15 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
> >               int curr = i++, err;
> >               int subi;
> >
> > -             if (!perf_test__matches(t->desc, curr, argc, argv)) {
> > +             if (!perf_test__matches(test_description(t, -1), curr, argc, argv)) {
> >                       bool skip = true;
> >                       int subn;
> >
> > -                     if (!t->subtest.get_nr)
> > -                             continue;
> > -
> > -                     subn = t->subtest.get_nr();
> > +                     subn = num_subtests(t);
>
> should you call continue on !subn ?

It's not necessary. When subn == 0 then the loop won't be taken and
skip == true, so the immediately after "if (skip) continue;" will
happen.

Thanks,
Ian

> jirka
>
> >
> >                       for (subi = 0; subi < subn; subi++) {
> > -                             if (perf_test__matches(t->subtest.get_desc(subi), curr, argc, argv))
> > +                             if (perf_test__matches(test_description(t, subi),
> > +                                                     curr, argc, argv))
> >                                       skip = false;
> >                       }
> >
> > @@ -425,22 +461,23 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
> >                               continue;
> >               }
> >
>
> SNIP
>