From patchwork Mon Jul 1 13:05:55 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 33497 Received: (qmail 122983 invoked by alias); 1 Jul 2019 13:06:06 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 122970 invoked by uid 89); 1 Jul 2019 13:06:04 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_STOCKGEN, RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=hits, HX-Google-Smtp-Source:APXvYqw, weeks, reverted X-HELO: mail-wm1-f65.google.com Received: from mail-wm1-f65.google.com (HELO mail-wm1-f65.google.com) (209.85.128.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 01 Jul 2019 13:05:59 +0000 Received: by mail-wm1-f65.google.com with SMTP id a15so15793527wmj.5 for ; Mon, 01 Jul 2019 06:05:58 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:4c97:6d52:2cea:997b? ([2001:8a0:f913:f700:4c97:6d52:2cea:997b]) by smtp.gmail.com with ESMTPSA id w25sm5853553wmk.18.2019.07.01.06.05.56 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Mon, 01 Jul 2019 06:05:56 -0700 (PDT) Subject: Re: ui_out format strings for fields and styles (Re: [PATCH] Style "pwd" output) To: Tom Tromey References: <20190605020116.1550-1-tom@tromey.com> <1ee4bd6b-4cdf-f3a9-74af-0843bf123a8b@redhat.com> <87lfygi1x0.fsf@tromey.com> <32872d6a-15d6-9718-59ae-957694e114c9@redhat.com> <87imtjhj6b.fsf@tromey.com> <625cd0ba-058d-d4bf-8ba3-8676f335b0f3@redhat.com> <87blzbep47.fsf@tromey.com> <2180f72f-da10-5333-90a1-666ba3bd145e@redhat.com> <87imtjbrmx.fsf@tromey.com> <871s056yjw.fsf@tromey.com> <87wohx5hir.fsf@tromey.com> <4e543ef2-eec3-b82c-a84a-a107e1ef2bc2@redhat.com> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <5df7a829-1cff-3df4-f2d3-92076cdc4699@redhat.com> Date: Mon, 1 Jul 2019 14:05:55 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <4e543ef2-eec3-b82c-a84a-a107e1ef2bc2@redhat.com> On 7/1/19 1:55 PM, Pedro Alves wrote: > On 7/1/19 1:23 PM, Pedro Alves wrote: >>> What's left to do on that branch in order to merge it? >> Well, you used printf_filtered the new patches, but that doesn't work >> with the new format specifiers, yet. :-) > > There are two other things that I wanted to experiment/try first. > > - See if we could get rid of the need for the ".ptr ()" calls. So those "ptr ()" calls/members are there because int_field/styled_string are ctors, and, we can't pass references via va_list. We can get rid of those, if we replace the ctors with functions that allocate a temporary on the callers stack, like: static inline styled_string_s * styled_string (const ui_file_style &style, const char *str, styled_string_s &&tmp = {}) { tmp.style = style; tmp.str = str; return &tmp; } Actually, what I originally thought to try, was to make it as short as possible to write that "styled_string" expression. I even experimented with renaming that styled_string function above to "ss", resulting in: printf_filtered (": file %ps, line %d.", ss (file_name_style.style (), filename), b->loc->line_number); but I reverted it, thinking that I was maybe overdoing it. We'll still need to use .ptr() with: fprintf_filtered (stream, " %pS%pS", metadata_style.style ().ptr (), reps, nullptr); (whatever the formatter spelling we end up with) unless we do make style() return a pointer. Here's a patch implementing the idea above. I wrote this a couple weeks ago, and at the time I felt more strongly about it. Is this worth it? From fd5bce2ea4716552ce9c3f9fdac7628cbf5a5cd2 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Fri, 7 Jun 2019 22:42:57 +0100 Subject: [PATCH] down with .ptr() --- gdb/breakpoint.c | 14 ++++++------ gdb/macrocmd.c | 2 +- gdb/printcmd.c | 2 +- gdb/symfile.c | 8 +++---- gdb/symtab.c | 6 +++--- gdb/ui-out.c | 8 +++---- gdb/ui-out.h | 65 ++++++++++++++++++++++++++++++-------------------------- 7 files changed, 54 insertions(+), 51 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 14ff32a68a0..77f416eb9ca 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -4930,7 +4930,7 @@ watchpoint_check (bpstat bs) uiout->message ("\nWatchpoint %pF deleted because the program has " "left the block in\n" "which its expression is valid.\n", - int_field ("wpnum", b->number).ptr ()); + int_field ("wpnum", b->number)); } /* Make sure the watchpoint's commands aren't executed. */ @@ -6246,7 +6246,7 @@ print_one_breakpoint_location (struct breakpoint *b, { annotate_field (8); uiout->message ("\tignore next %pF hits\n", - int_field ("ignore", b->ignore_count).ptr ()); + int_field ("ignore", b->ignore_count)); } /* Note that an enable count of 1 corresponds to "enable once" @@ -6695,7 +6695,7 @@ describe_other_breakpoints (struct gdbarch *gdbarch, } current_uiout->message (_("also set at pc %ps.\n"), styled_string (address_style.style (), - paddress (gdbarch, pc)).ptr ()); + paddress (gdbarch, pc))); } } @@ -12115,7 +12115,7 @@ say_where (struct breakpoint *b) printf_filtered (" at %ps", styled_string (address_style.style (), paddress (b->loc->gdbarch, - b->loc->address)).ptr ()); + b->loc->address))); if (b->loc->symtab != NULL) { /* If there is a single location, we can print the location @@ -12126,7 +12126,7 @@ say_where (struct breakpoint *b) = symtab_to_filename_for_display (b->loc->symtab); printf_filtered (": file %ps, line %d.", styled_string (file_name_style.style (), - filename).ptr (), + filename), b->loc->line_number); } else @@ -12433,10 +12433,10 @@ bkpt_print_it (bpstat bs) } if (bp_temp) uiout->message ("Temporary breakpoint %pF, ", - int_field ("bkptno", b->number).ptr ()); + int_field ("bkptno", b->number)); else uiout->message ("Breakpoint %pF, ", - int_field ("bkptno", b->number).ptr ()); + int_field ("bkptno", b->number)); return PRINT_SRC_AND_LOC; } diff --git a/gdb/macrocmd.c b/gdb/macrocmd.c index 0f1f239c5a5..cb7267d07a3 100644 --- a/gdb/macrocmd.c +++ b/gdb/macrocmd.c @@ -122,7 +122,7 @@ show_pp_source_pos (struct ui_file *stream, std::string fullname = macro_source_fullname (file); fprintf_filtered (stream, "%ps:%d\n", styled_string (file_name_style.style (), - fullname.c_str ()).ptr (), + fullname.c_str ()), line); while (file->included_by) diff --git a/gdb/printcmd.c b/gdb/printcmd.c index 0aa9ac07819..61deb37c349 100644 --- a/gdb/printcmd.c +++ b/gdb/printcmd.c @@ -2190,7 +2190,7 @@ print_variable_and_value (const char *name, struct symbol *var, name = SYMBOL_PRINT_NAME (var); fprintf_filtered (stream, "%s%ps = ", n_spaces (2 * indent), - styled_string (variable_name_style.style (), name).ptr ()); + styled_string (variable_name_style.style (), name)); try { diff --git a/gdb/symfile.c b/gdb/symfile.c index 903f2ff817b..03f9013315c 100644 --- a/gdb/symfile.c +++ b/gdb/symfile.c @@ -1117,7 +1117,7 @@ symbol_file_add_with_addrs (bfd *abfd, const char *name, else current_uiout->message (_("Reading symbols from %ps...\n"), styled_string (file_name_style.style (), - name).ptr ()); + name)); } syms_from_objfile (objfile, addrs, add_flags); @@ -1130,8 +1130,7 @@ symbol_file_add_with_addrs (bfd *abfd, const char *name, { if (should_print) current_uiout->message (_("Expanding full symbols from %ps...\n"), - styled_string (file_name_style.style (), - name).ptr ()); + styled_string (file_name_style.style (), name)); if (objfile->sf) objfile->sf->qf->expand_all_symtabs (objfile); @@ -1144,8 +1143,7 @@ symbol_file_add_with_addrs (bfd *abfd, const char *name, if (should_print && !objfile_has_symbols (objfile) && objfile->separate_debug_objfile == nullptr) current_uiout->message (_("(No debugging symbols found in %ps)\n"), - styled_string (file_name_style.style (), - name).ptr ()); + styled_string (file_name_style.style (), name)); if (should_print) { diff --git a/gdb/symtab.c b/gdb/symtab.c index e5f702ab804..e6253604c5f 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -4605,7 +4605,7 @@ print_symbol_info (enum search_domain kind, { current_uiout->message (_("\nFile %ps:\n"), - styled_string (file_name_style.style (), s_filename).ptr ()); + styled_string (file_name_style.style (), s_filename)); } if (SYMBOL_LINE (sym) != 0) @@ -4658,8 +4658,8 @@ print_msymbol_info (struct bound_minimal_symbol msymbol) current_uiout->message (_("%ps %ps\n"), - styled_string (address_style.style (), tmp).ptr (), - styled_string (sym_style, MSYMBOL_PRINT_NAME (msymbol.minsym)).ptr ()); + styled_string (address_style.style (), tmp), + styled_string (sym_style, MSYMBOL_PRINT_NAME (msymbol.minsym))); } /* This is the guts of the commands "info functions", "info types", and diff --git a/gdb/ui-out.c b/gdb/ui-out.c index 25efa98d56a..92e461f850e 100644 --- a/gdb/ui-out.c +++ b/gdb/ui-out.c @@ -607,14 +607,14 @@ ui_out::vmessage (const char *format, va_list args) { case 'F': { - int_field *field = va_arg (args, int_field *); - field_int (field->name (), field->val ()); + int_field_s *field = va_arg (args, int_field_s *); + field_int (field->name, field->val); } break; case 's': { - styled_string *ss = va_arg (args, styled_string *); - call_do_message (ss->style (), "%s", ss->str ()); + styled_string_s *ss = va_arg (args, styled_string_s *); + call_do_message (ss->style, "%s", ss->str); } break; case 'S': diff --git a/gdb/ui-out.h b/gdb/ui-out.h index 35bc8b21824..fbf9c9bd326 100644 --- a/gdb/ui-out.h +++ b/gdb/ui-out.h @@ -68,45 +68,50 @@ enum ui_out_type ui_out_type_list }; -struct int_field +/* An int field, to be passed to %pF in format strings. */ + +struct int_field_s { - int_field (const char *name, int val) - : m_name (name), - m_val (val) - { - } + const char *name; + int val; +}; - /* We need this because we can't pass a reference via - va_args. */ - const int_field *ptr () const { return this; } +/* Construct a temporary int_field_s on the caller's stack and return + a pointer to the constructed object. We use this because it's not + possible to pass a reference via va_args. */ - const char *name () const {return m_name; } - int val () const {return m_val; } +static inline int_field_s * +int_field (const char *name, int val, + int_field_s &&tmp = {}) +{ + tmp.name = name; + tmp.val = val; + return &tmp; +} -private: - const char *m_name; - int m_val; -}; +/* A styled string. */ -struct styled_string +struct styled_string_s { - styled_string (const ui_file_style &style, const char *str) - : m_style (style), - m_str (str) - { - } + /* The style. */ + ui_file_style style; - /* We need this because we can't pass a reference via - va_args. */ - const styled_string *ptr () const { return this; } + /* The string. */ + const char *str; +}; - const ui_file_style &style () const { return m_style; } - const char *str () const {return m_str; } +/* Construct a temporary styled_string_s on the caller's stack and + return a pointer to the constructed object. We use this because + it's not possible to pass a reference via va_args. */ -private: - ui_file_style m_style; - const char *m_str; -}; +static inline styled_string_s * +styled_string (const ui_file_style &style, const char *str, + styled_string_s &&tmp = {}) +{ + tmp.style = style; + tmp.str = str; + return &tmp; +} class ui_out {