From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 33380 invoked by alias); 29 Apr 2015 15:43:53 -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 33368 invoked by uid 89); 29 Apr 2015 15:43:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Wed, 29 Apr 2015 15:43:51 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t3TFhoKe002511 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Wed, 29 Apr 2015 11:43:50 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t3TFhmfD011828; Wed, 29 Apr 2015 11:43:49 -0400 Message-ID: <5540FC34.3050200@redhat.com> Date: Wed, 29 Apr 2015 15:44:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Jan Kratochvil , gdb-patches@sourceware.org CC: Phil Muldoon Subject: Re: [PATCH v3 1/9] Code cleanup: Make parts of print_command_1 public References: <20150411194322.29128.52477.stgit@host1.jankratochvil.net> <20150411194333.29128.30245.stgit@host1.jankratochvil.net> In-Reply-To: <20150411194333.29128.30245.stgit@host1.jankratochvil.net> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-04/txt/msg01071.txt.bz2 On 04/11/2015 08:43 PM, Jan Kratochvil wrote: > +/* Parse print command format string and update *EXPP, return it allocated, > + caller has to call xfree for it. Return NULL if no format string has been > + found. CMDNAME should name the current command. */ > + > +struct format_data * > +print_command_parse_format (const char **expp, const char *cmdname) > +{ I read the series, and AFAICS, this will be used by compile_print_command in patch #7. But then AFAICS, compile_print_command leaks fmtp. I think this all ends up simpler if it follows the pattern that the current code already follows. That is, instead of having print_command_parse_format "struct format_data" on the heap, make the function take an output format parameter: void print_command_parse_format (const char **expp, const char *cmdname, struct format_data *fmt) { > + struct format_data *fmtp; > + const char *exp = *expp; > + struct cleanup *cleanup; > + > + if (exp == NULL || *exp != '/') > + return NULL; and in this case set the defaults in the passed in fmt, like the original code does: > - if (exp && *exp == '/') > - { ... > - } > - else > - { > - fmt.count = 1; > - fmt.format = 0; > - fmt.size = 0; > - fmt.raw = 0; > - } (the else branch) > + exp++; > + > + fmtp = xmalloc (sizeof (*fmtp)); > + cleanup = make_cleanup (xfree, fmtp); > + *fmtp = decode_format (&exp, last_format, 0); > + validate_format (*fmtp, cmdname); > + last_format = fmtp->format; > + > + discard_cleanups (cleanup); ... and then no need for the xmalloc and cleanups... > + *expp = exp; > + return fmtp; > +} > + > + > +/* Print VAL to console, including recording it to the history. */ > + > +void > +print_value (struct value *val, const struct format_data *fmtp) > +{ > + struct value_print_options opts; > + int histindex = record_latest_value (val); > + > + annotate_value_history_begin (histindex, value_type (val)); > + > + printf_filtered ("$%d = ", histindex); > + > + annotate_value_history_value (); > + > + get_formatted_print_options (&opts, (fmtp == NULL ? 0 : fmtp->format)); > + opts.raw = (fmtp == NULL ? 0 : fmtp->raw); > + > + print_formatted (val, (fmtp == NULL ? 0 : fmtp->size), &opts, gdb_stdout); ... and no need for the NULL checks here... > + printf_filtered ("\n"); > + > + annotate_value_history_end (); > +} > + > /* Evaluate string EXP as an expression in the current language and > print the resulting value. EXP may contain a format specifier as the > first argument ("/x myvar" for example, to print myvar in hex). */ > @@ -947,25 +997,9 @@ static void > print_command_1 (const char *exp, int voidprint) > { > struct expression *expr; > - struct cleanup *old_chain = make_cleanup (null_cleanup, NULL); > - char format = 0; > struct value *val; > - struct format_data fmt; ... and this 'fmt' variable stays on the stack ... > - > - if (exp && *exp == '/') > - { > - exp++; > - fmt = decode_format (&exp, last_format, 0); > - validate_format (fmt, "print"); > - last_format = format = fmt.format; > - } > - else > - { > - fmt.count = 1; > - fmt.format = 0; > - fmt.size = 0; > - fmt.raw = 0; > - } > + struct format_data *fmtp = print_command_parse_format (&exp, "print"); > + struct cleanup *old_chain = make_cleanup (xfree, fmtp); ... and no need for this cleanup either. > > if (exp && *exp) > { > @@ -978,24 +1012,7 @@ print_command_1 (const char *exp, int voidprint) > > if (voidprint || (val && value_type (val) && > TYPE_CODE (value_type (val)) != TYPE_CODE_VOID)) > - { > - struct value_print_options opts; > - int histindex = record_latest_value (val); > - > - annotate_value_history_begin (histindex, value_type (val)); > - > - printf_filtered ("$%d = ", histindex); > - > - annotate_value_history_value (); > - > - get_formatted_print_options (&opts, format); > - opts.raw = fmt.raw; > - > - print_formatted (val, fmt.size, &opts, gdb_stdout); > - printf_filtered ("\n"); > - > - annotate_value_history_end (); > - } > + print_value (val, fmtp); > > do_cleanups (old_chain); > } > diff --git a/gdb/valprint.h b/gdb/valprint.h > index e3d0137..3ab531f 100644 > --- a/gdb/valprint.h > +++ b/gdb/valprint.h > @@ -217,4 +217,9 @@ extern void output_command_const (const char *args, int from_tty); > > extern int val_print_scalar_type_p (struct type *type); > > +struct format_data; > +extern struct format_data *print_command_parse_format (const char **expp, > + const char *cmdname); > +extern void print_value (struct value *val, const struct format_data *fmtp); > + > #endif > Thanks, Pedro Alves