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

[Gustavo Sousa]

Quoting Kamil Konieczny (2024-05-10 14:37:26-03:00)
>Hi Gustavo,
>On 2024-04-30 at 18:19:35 -0300, 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
>
>I do not know if 'igt@' was always used in tests lists.

Hm... Not sure I get the point here.

Regardless, (delim == NULL) is an impossible condition at the points
this patch is touching, no?

>Adding Petri to cc.
>+Cc: Petri Latvala <adrinael at adrinael.net>

Thanks!

--
Gustavo Sousa

>
>Regards,
>Kamil
>
>> 
>>     $ ./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>
>> ---
>>  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);
>> -- 
>> 2.44.0
>>