[i-g-t] runner: Remove useless null check for delim in job_list_from_test_list

[Matt Roper]

On Tue, May 07, 2024 at 10:13:45AM -0700, Ngai-Mint Kwan wrote:
> Hi Gustavo,
> 
> On 2024-04-30 14:19, Gustavo Sousa wrote:
> > The function job_list_from_test_list() uses a while loop to read entries
> > from the testlist.
> > 
> > The condition ((delim = strchr(binary, '@')) != NULL) checks if the
> > current entry specifies a subtest. When no subtest is specified, the
> > else clause will cause a jump to the next iteration. That means that we
> > are certain any statement executed after that if block has (delim !=
> > NULL). As such, all null checks on that variable after that point are
> > pointless.
> > 
> > In fact, certain unnecessary null checks might even cause confusion to
> > readers. One example is the block meant to display the "Unexpected test
> > without subtests ..." message: it would never happen, since that
> > condition is handled earlier in the iteration (i.e. the full set of
> > subtests is expanded for that entry in the else clause previously
> > mentioned). The following execution (done before this change)
> > illustrates that:
> > 
> >      $ cat <<EOF > /tmp/foo.testlist
> >      igt at xe_pm@s2idle-basic
> >      igt at xe_pm
> >      igt at xe_pm@s2idle-exec-after
> >      EOF
> > 
> >      $ ./build/runner/igt_runner -d -m --test-list /tmp/foo.testlist build/tests /tmp/results
> >      [36408.109043] Dry run, not executing. Invoke igt_resume if you want to execute.
> >      Done.
> > 
> >      $ cat /tmp/results/joblist.txt | sed 's/^\(.\{50\}\).\+/\1.../'
> >      xe_pm s2idle-basic
> >      xe_pm s2idle-basic,s2idle-basic-exec,s2idle-exec-a...
> >      xe_pm s2idle-exec-after
> > 
> > Let's remove those unnecessary checks.
> > 
> > Signed-off-by: Gustavo Sousa <gustavo.sousa at intel.com>
> 
> Reviewed-by: Ngai-Mint Kwan <ngai-mint.kwan at linux.intel.com>
> 
> Thanks,
> Ngai-Mint Kwan

Applied, thanks for the patch and review.


Matt

> 
> > ---
> >   runner/job_list.c | 21 +++++----------------
> >   1 file changed, 5 insertions(+), 16 deletions(-)
> > 
> > diff --git a/runner/job_list.c b/runner/job_list.c
> > index 27cbb10bce56..aeabb59f49d3 100644
> > --- a/runner/job_list.c
> > +++ b/runner/job_list.c
> > @@ -276,16 +276,7 @@ static bool job_list_from_test_list(struct job_list *job_list,
> >   			 * specified, also start a new entry.
> >   			 */
> >   			if (entry.binary && !strcmp(entry.binary, binary) &&
> > -			    (delim == NULL || strchr(delim, '@') == NULL)) {
> > -				if (!delim) {
> > -					/* ... except we didn't get a subtest */
> > -					fprintf(stderr,
> > -						"Error: Unexpected test without subtests "
> > -						"after same test had subtests\n");
> > -					free(binary);
> > -					fclose(f);
> > -					return false;
> > -				}
> > +			    strchr(delim, '@') == NULL) {
> >   				entry.subtest_count++;
> >   				entry.subtests = realloc(entry.subtests,
> >   							 entry.subtest_count *
> > @@ -303,7 +294,7 @@ static bool job_list_from_test_list(struct job_list *job_list,
> >   			memset(&entry, 0, sizeof(entry));
> > -			if (delim != NULL && strchr(delim, '@') != NULL) {
> > +			if (strchr(delim, '@') != NULL) {
> >   				/* Dynamic subtest specified. Add to job list alone. */
> >   				char **subtests;
> > @@ -314,11 +305,9 @@ static bool job_list_from_test_list(struct job_list *job_list,
> >   				any = true;
> >   			} else {
> >   				entry.binary = strdup(binary);
> > -				if (delim) {
> > -					entry.subtests = malloc(sizeof(*entry.subtests));
> > -					entry.subtests[0] = strdup(delim);
> > -					entry.subtest_count = 1;
> > -				}
> > +				entry.subtests = malloc(sizeof(*entry.subtests));
> > +				entry.subtests[0] = strdup(delim);
> > +				entry.subtest_count = 1;
> >   			}
> >   			free(binary);
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation