From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 90299 invoked by alias); 29 Jul 2017 23:10:54 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 90286 invoked by uid 89); 29 Jul 2017 23:10:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=tsv, HTo:U*tom, enb, agent X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sat, 29 Jul 2017 23:10:52 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id v6TNAj2r008624 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Sat, 29 Jul 2017 19:10:50 -0400 Received: by simark.ca (Postfix, from userid 112) id B7A891EA05; Sat, 29 Jul 2017 19:10:45 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id C8BB91E043; Sat, 29 Jul 2017 19:10:43 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Sat, 29 Jul 2017 23:10:00 -0000 From: Simon Marchi To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [RFA v2 01/24] Introduce and use ui_out_emit_table In-Reply-To: <20170725172107.9799-2-tom@tromey.com> References: <20170725172107.9799-1-tom@tromey.com> <20170725172107.9799-2-tom@tromey.com> Message-ID: X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Sat, 29 Jul 2017 23:10:45 +0000 X-IsSubscribed: yes X-SW-Source: 2017-07/txt/msg00435.txt.bz2 Hi Tom, Just some formatting comments, I think it's fine to push with those fixed. > @@ -6851,48 +6850,43 @@ breakpoint_1 (char *args, int allflag, > } > } > > - if (opts.addressprint) > - bkpttbl_chain > - = make_cleanup_ui_out_table_begin_end (uiout, 6, > - nr_printable_breakpoints, > - "BreakpointTable"); > - else > - bkpttbl_chain > - = make_cleanup_ui_out_table_begin_end (uiout, 5, > - nr_printable_breakpoints, > - "BreakpointTable"); > - > - if (nr_printable_breakpoints > 0) > - annotate_breakpoints_headers (); > - if (nr_printable_breakpoints > 0) > - annotate_field (0); > - uiout->table_header (7, ui_left, "number", "Num"); /* 1 */ > - if (nr_printable_breakpoints > 0) > - annotate_field (1); > - uiout->table_header (print_type_col_width, ui_left, "type", "Type"); > /* 2 */ > - if (nr_printable_breakpoints > 0) > - annotate_field (2); > - uiout->table_header (4, ui_left, "disp", "Disp"); /* 3 */ > - if (nr_printable_breakpoints > 0) > - annotate_field (3); > - uiout->table_header (3, ui_left, "enabled", "Enb"); /* 4 */ > - if (opts.addressprint) > - { > - if (nr_printable_breakpoints > 0) > - annotate_field (4); > - if (print_address_bits <= 32) > - uiout->table_header (10, ui_left, "addr", "Address"); /* 5 */ > - else > - uiout->table_header (18, ui_left, "addr", "Address"); /* 5 */ > - } > - if (nr_printable_breakpoints > 0) > - annotate_field (5); > - uiout->table_header (40, ui_noalign, "what", "What"); /* 6 */ > - uiout->table_body (); > - if (nr_printable_breakpoints > 0) > - annotate_breakpoints_table (); > + { > + ui_out_emit_table table_emitter (uiout, > + opts.addressprint ? 6 : 5, > + nr_printable_breakpoints, > + "BreakpointTable"); > + > + if (nr_printable_breakpoints > 0) > + annotate_breakpoints_headers (); > + if (nr_printable_breakpoints > 0) > + annotate_field (0); > + uiout->table_header (7, ui_left, "number", "Num"); /* 1 */ > + if (nr_printable_breakpoints > 0) > + annotate_field (1); > + uiout->table_header (print_type_col_width, ui_left, "type", > "Type"); /* 2 */ > + if (nr_printable_breakpoints > 0) > + annotate_field (2); > + uiout->table_header (4, ui_left, "disp", "Disp"); /* 3 */ > + if (nr_printable_breakpoints > 0) > + annotate_field (3); > + uiout->table_header (3, ui_left, "enabled", "Enb"); /* 4 */ > + if (opts.addressprint) > + { > + if (nr_printable_breakpoints > 0) > + annotate_field (4); > + if (print_address_bits <= 32) > + uiout->table_header (10, ui_left, "addr", "Address"); /* 5 */ > + else > + uiout->table_header (18, ui_left, "addr", "Address"); /* 5 */ > + } > + if (nr_printable_breakpoints > 0) > + annotate_field (5); > + uiout->table_header (40, ui_noalign, "what", "What"); /* 6 */ > + uiout->table_body (); > + if (nr_printable_breakpoints > 0) > + annotate_breakpoints_table (); > > - ALL_BREAKPOINTS (b) > + ALL_BREAKPOINTS (b) Shouldn't the scope just under this be indented as well? > @@ -1075,19 +1074,18 @@ info_sharedlibrary_command (char *pattern, int > from_tty) > } > } > > - table_cleanup = > - make_cleanup_ui_out_table_begin_end (uiout, 4, nr_libs, > - "SharedLibraryTable"); > + { > + ui_out_emit_table table_emitter (uiout, 4, nr_libs, > "SharedLibraryTable"); > > - /* The "- 1" is because ui_out adds one space between columns. */ > - uiout->table_header (addr_width - 1, ui_left, "from", "From"); > - uiout->table_header (addr_width - 1, ui_left, "to", "To"); > - uiout->table_header (12 - 1, ui_left, "syms-read", "Syms Read"); > - uiout->table_header (0, ui_noalign, "name", "Shared Object > Library"); > + /* The "- 1" is because ui_out adds one space between columns. */ > + uiout->table_header (addr_width - 1, ui_left, "from", "From"); > + uiout->table_header (addr_width - 1, ui_left, "to", "To"); > + uiout->table_header (12 - 1, ui_left, "syms-read", "Syms Read"); > + uiout->table_header (0, ui_noalign, "name", "Shared Object > Library"); > > - uiout->table_body (); > + uiout->table_body (); > > - ALL_SO_LIBS (so) > + ALL_SO_LIBS (so) I think the scope below should be indented as well. > { > if (! so->so_name[0]) > continue; > @@ -1121,8 +1119,7 @@ info_sharedlibrary_command (char *pattern, int > from_tty) > > uiout->text ("\n"); > } > - > - do_cleanups (table_cleanup); > + } > > if (nr_libs == 0) > { > diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c > index 4f2bac5..6721e22 100644 > --- a/gdb/tracepoint.c > +++ b/gdb/tracepoint.c > @@ -482,7 +482,6 @@ tvariables_info_1 (void) > struct trace_state_variable *tsv; > int ix; > int count = 0; > - struct cleanup *back_to; > struct ui_out *uiout = current_uiout; > > if (VEC_length (tsv_s, tvariables) == 0 && !uiout->is_mi_like_p ()) > @@ -496,8 +495,7 @@ tvariables_info_1 (void) > tsv->value_known = target_get_trace_state_variable_value > (tsv->number, > &(tsv->value)); > > - back_to = make_cleanup_ui_out_table_begin_end (uiout, 3, > - count, > "trace-variables"); > + ui_out_emit_table table_emitter (uiout, 3, count, > "trace-variables"); > uiout->table_header (15, ui_left, "name", "Name"); > uiout->table_header (11, ui_left, "initial", "Initial"); > uiout->table_header (11, ui_left, "current", "Current"); > @@ -531,8 +529,6 @@ tvariables_info_1 (void) > uiout->field_string ("current", c); > uiout->text ("\n"); > } > - > - do_cleanups (back_to); > } > > /* List all the trace state variables. */ > @@ -3952,9 +3948,8 @@ info_static_tracepoint_markers_command (char > *arg, int from_tty) > don't work without in-process agent, so we don't bother users to > type > `set agent on' when to use static tracepoint. */ > > - old_chain > - = make_cleanup_ui_out_table_begin_end (uiout, 5, -1, > - "StaticTracepointMarkersTable"); > + ui_out_emit_table table_emitter (uiout, 5, -1, > + "StaticTracepointMarkersTable"); > > uiout->table_header (7, ui_left, "counter", "Cnt"); > > @@ -3970,7 +3965,7 @@ info_static_tracepoint_markers_command (char > *arg, int from_tty) > uiout->table_body (); > > markers = target_static_tracepoint_markers_by_strid (NULL); > - make_cleanup (VEC_cleanup (static_tracepoint_marker_p), &markers); > + old_chain = make_cleanup (VEC_cleanup (static_tracepoint_marker_p), > &markers); > > for (i = 0; > VEC_iterate (static_tracepoint_marker_p, > diff --git a/gdb/ui-out.h b/gdb/ui-out.h > index 9278cab..2293f26 100644 > --- a/gdb/ui-out.h > +++ b/gdb/ui-out.h > @@ -220,4 +220,31 @@ private: > typedef ui_out_emit_type ui_out_emit_tuple; > typedef ui_out_emit_type ui_out_emit_list; > > +/* This is similar to make_cleanup_ui_out_table_begin_end, but written > + as an RAII class. */ > +class ui_out_emit_table > +{ > +public: > + > + ui_out_emit_table (struct ui_out *uiout, int nr_cols, int nr_rows, > + const char *tblid) > + Spurious empty line? > + : m_uiout (uiout) > + { > + m_uiout->table_begin (nr_cols, nr_rows, tblid); > + } > + > + ~ui_out_emit_table () > + { > + m_uiout->table_end (); > + } > + > + ui_out_emit_table (const ui_out_emit_table &) = delete; > + ui_out_emit_table &operator= (const ui_out_emit_table &) = delete; > + > +private: > + > + struct ui_out *m_uiout; > +}; > + > #endif /* UI_OUT_H */ Thanks, Simon