Re: [RFA 1/7] Use ui_out_emit_table and ui_out_emit_list in print_thread_info_1

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2017-09-09 17:35, Tom Tromey wrote:
> This changes print_thread_info_1 to use ui_out_emit_table and
> ui_out_emit_list.  Which one is used depends on whether the ui-out is
> mi-like; so the emitters are wrapped in gdb::optional.

LGTM.

I think overall this function is a bad example of how to share code 
between CLI and MI.  There are so many if (is_mi_like_p) that it's 
essentially two functions in one.  Apart from iterating on threads, the 
MI and CLI outputs don't share much...

> @@ -1247,55 +1248,55 @@ print_thread_info_1 (struct ui_out *uiout,
> char *requested_threads,
>    update_thread_list ();
>    current_ptid = inferior_ptid;
>
> -  struct cleanup *old_chain = make_cleanup (null_cleanup, NULL);
> -
> -  /* For backward compatibility, we make a list for MI.  A table is
> -     preferable for the CLI, though, because it shows table
> -     headers.  */
> -  if (uiout->is_mi_like_p ())
> -    make_cleanup_ui_out_list_begin_end (uiout, "threads");
> -  else
> -    {
> -      int n_threads = 0;
> +  {
> +    /* For backward compatibility, we make a list for MI.  A table is
> +       preferable for the CLI, though, because it shows table
> +       headers.  */
> +    gdb::optional<ui_out_emit_list> list_emitter;
> +    gdb::optional<ui_out_emit_table> table_emitter;
> +
> +    if (uiout->is_mi_like_p ())
> +      list_emitter.emplace (uiout, "threads");
> +    else
> +      {
> +	int n_threads = 0;
>
> -      for (tp = thread_list; tp; tp = tp->next)
> -	{
> -	  if (!should_print_thread (requested_threads, default_inf_num,
> -				    global_ids, pid, tp))
> -	    continue;
> +	for (tp = thread_list; tp; tp = tp->next)
> +	  {
> +	    if (!should_print_thread (requested_threads, default_inf_num,
> +				      global_ids, pid, tp))
> +	      continue;
>
> -	  ++n_threads;
> -	}
> +	    ++n_threads;
> +	  }
>
> -      if (n_threads == 0)
> -	{
> -	  if (requested_threads == NULL || *requested_threads == '\0')
> -	    uiout->message (_("No threads.\n"));
> -	  else
> -	    uiout->message (_("No threads match '%s'.\n"),
> -			    requested_threads);
> -	  do_cleanups (old_chain);
> -	  return;
> -	}
> +	if (n_threads == 0)
> +	  {
> +	    if (requested_threads == NULL || *requested_threads == '\0')
> +	      uiout->message (_("No threads.\n"));
> +	    else
> +	      uiout->message (_("No threads match '%s'.\n"),
> +			      requested_threads);
> +	    return;
> +	  }
>
> -      if (show_global_ids || uiout->is_mi_like_p ())
> -	make_cleanup_ui_out_table_begin_end (uiout, 5, n_threads, "threads");
> -      else
> -	make_cleanup_ui_out_table_begin_end (uiout, 4, n_threads, "threads");
> +	table_emitter.emplace (uiout,
> +			       (show_global_ids || uiout->is_mi_like_p ())
> +			       ? 5 : 4,
> +			       n_threads, "threads");
>
> -      uiout->table_header (1, ui_left, "current", "");
> +	uiout->table_header (1, ui_left, "current", "");
>
> -      if (!uiout->is_mi_like_p ())
> -	uiout->table_header (4, ui_left, "id-in-tg", "Id");
> -      if (show_global_ids || uiout->is_mi_like_p ())
> -	uiout->table_header (4, ui_left, "id", "GId");
> -      uiout->table_header (17, ui_left, "target-id", "Target Id");
> -      uiout->table_header (1, ui_left, "frame", "Frame");
> -      uiout->table_body ();
> -    }
> +	if (!uiout->is_mi_like_p ())
> +	  uiout->table_header (4, ui_left, "id-in-tg", "Id");
> +	if (show_global_ids || uiout->is_mi_like_p ())
> +	  uiout->table_header (4, ui_left, "id", "GId");
> +	uiout->table_header (17, ui_left, "target-id", "Target Id");
> +	uiout->table_header (1, ui_left, "frame", "Frame");
> +	uiout->table_body ();
> +      }

Actually, we could remove a lot of if (is_mi_like_p) from here, since 
they are already in an else branch of the same check.  I'll wait until 
this patch has been merged to clean that up, so you don't have a 
conflict.

Thanks,

Simon