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

  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.

ChangeLog
2017-09-09  Tom Tromey  <tom@tromey.com>

	* thread.c (print_thread_info_1): Use ui_out_emit_table,
	ui_out_emit_list, gdb::optional.
---
 gdb/ChangeLog |  5 ++++
 gdb/thread.c  | 92 +++++++++++++++++++++++++++++------------------------------
 2 files changed, 51 insertions(+), 46 deletions(-)
  

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
  

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

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

I think a long term goal should be to remove all those is_mi_like_p
checks.  In some situations this might mean introducing "MI 4" and
fixing up the historical baggage; but that would also be a good thing,
as there are a few places where gdb has remained buggy for the sake of
not breaking existing MI readers.

What I'd really like is for MI to be defined as JSON (it is pretty close
already), but that's maybe a bit much to ask.

A related thought is that if more spots used ui-out (I'm thinking mainly
*-valprint, but maybe there are others), then colorization could be done
at the ui-out layer.

Tom
  

On Sat, Sep 9, 2017 at 11:36 AM, Tom Tromey <tom@tromey.com> wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
>
> Simon> I think overall this function is a bad example of how to share code
> Simon> between CLI and MI.  There are so many if (is_mi_like_p) that it's
> Simon> essentially two functions in one.  Apart from iterating on threads,
> Simon> the MI and CLI outputs don't share much...
>
> I think a long term goal should be to remove all those is_mi_like_p
> checks.  In some situations this might mean introducing "MI 4" and
> fixing up the historical baggage; but that would also be a good thing,
> as there are a few places where gdb has remained buggy for the sake of
> not breaking existing MI readers.
>
> What I'd really like is for MI to be defined as JSON (it is pretty close
> already), but that's maybe a bit much to ask.

At some point I had it encoding into python literals, with MI output
being converted into something parsable by the python literal_eval function[1]

JSON is really not very different, the only thing which kind of sucked
about it was dealing with
the places a range of values, or a slice is sent, these needed the
range to then be marshalled and unmarshalled
into tuples of numbers then back into ranges, this is more of an
annoyance than anything catastrophic,
but it means you have to evaluate the literals, then evaluate the
objects returned by that first pass...
using some little snippet of code specific to the data being sent.

https://docs.python.org/2/library/ast.html#ast.literal_eval

I don't know JSON well, but i don't see its format having a notion of
slices either
neither do I remember where specifically this came up in mi,
but if both having and eating cake were on the table JSON + primitive
syntax for range/slices would
be nice.

> A related thought is that if more spots used ui-out (I'm thinking mainly
> *-valprint, but maybe there are others), then colorization could be done
> at the ui-out layer.
>
> Tom