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

Simon Marchi
2017-09-09T17:47:13Z

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