Re: [PATCH] Minor Ada task cleanups

[Joel Brobecker <brobecker at adacore dot com>]

Hi Tom,

On Fri, Feb 15, 2019 at 02:22:53PM -0700, Tom Tromey wrote:
> While working on the Ada task code, I noticed a few things that could
> be cleaned up:
> 
> * task_list_valid_p was not set in all cases in ada_build_task_list.
>   This causes many needless re-fetches of the task list.
> 
> * task_list_valid_p can be bool, and various functions can also return
>   bool.
> 
> * Nothing checks the return value of read_known_tasks, so it can be
>   changed to return void.
> 
> * The call to ada_build_task_list in
>   ravenscar_thread_target::update_thread_list is redundant, because
>   this is the first thing done by iterate_over_live_ada_tasks.
> 
> Tested using the internal AdaCore test suite against a ravenscar
> target.

Thanks for doing this. Nice cleanup :).

One question independent of this patch first: I am wondering - how
do you deal with submissions that you want to "git am" but have
a ChangeLog patch in it that cause a merge failure? I've needed to
apply your patch a few times, and the only option I knew was to
remove the hungs for the ChangeLog files.

> gdb/ChangeLog
> 2019-02-15  Tom Tromey  <tromey@adacore.com>
> 
> 	* ravenscar-thread.c
> 	(ravenscar_thread_target::update_thread_list): Don't call
> 	ada_build_task_list.
> 	* ada-lang.h (ada_build_task_list): Don't declare.
> 	* ada-tasks.c (struct ada_tasks_inferior_data)
> 	<task_list_valid_p>: Now bool.
> 	(read_known_tasks, ada_task_list_changed)
> 	(ada_tasks_invalidate_inferior_data): Update.
> 	(read_known_tasks_array): Return bool.
> 	(read_known_tasks_list): Likewise.
> 	(read_known_tasks): Return void.
> 	(ada_build_task_list): Now static.

Since this code is used on all platforms and native platforms
in particular have a lot more testcases that exercise this code,
it would be nice to exercise it both with the build-bot and
AdaCore's testsuite.

One minor nit below.

Other than that, the patch looks good to me; if the patch with
the change I suggested passes testing, it's OK to push.

Thank you!


> +static void
> +read_known_tasks ()
>  {
>    struct ada_tasks_inferior_data *data =
>      get_ada_tasks_inferior_data (current_inferior ());
> @@ -965,29 +966,30 @@ read_known_tasks (void)
>    ada_tasks_inferior_data_sniffer (data);
>    gdb_assert (data->known_tasks_kind != ADA_TASKS_UNKNOWN);
>  
> +  /* Step 3: Set task_list_valid_p, to avoid re-reading the Known_Tasks
> +     array unless needed.  */
>    switch (data->known_tasks_kind)
>      {
> -      case ADA_TASKS_NOT_FOUND: /* Tasking not in use in inferior.  */
> -        return 0;
> -      case ADA_TASKS_ARRAY:
> -        return read_known_tasks_array (data);
> -      case ADA_TASKS_LIST:
> -        return read_known_tasks_list (data);
> +    case ADA_TASKS_NOT_FOUND: /* Tasking not in use in inferior.  */
> +      break;
> +    case ADA_TASKS_ARRAY:
> +      data->task_list_valid_p = read_known_tasks_array (data);
> +      break;
> +    case ADA_TASKS_LIST:
> +      data->task_list_valid_p = read_known_tasks_list (data);
> +      break;
> +    case ADA_TASKS_UNKNOWN:
> +      data->task_list_valid_p = true;
> +      break;

The last case is unnecessary, as we have just asserted above
that data->known_tasks_kind != ADA_TASKS_UNKNOWN.

-- 
Joel