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

[Ngai-Mint Kwan <ngai-mint.kwan@linux.intel.com>]

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@xe_pm@s2idle-basic
>      igt@xe_pm
>      igt@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@intel.com>

Reviewed-by: Ngai-Mint Kwan <ngai-mint.kwan@linux.intel.com>

Thanks,
Ngai-Mint Kwan

> ---
>   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);