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

[Kamil Konieczny]

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.
Adding Petri to cc.
+Cc: Petri Latvala <adrinael at adrinael.net>

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
>