This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3 1/9] Code cleanup: Make parts of print_command_1 public
- From: Pedro Alves <palves at redhat dot com>
- To: Jan Kratochvil <jan dot kratochvil at redhat dot com>, gdb-patches at sourceware dot org
- Cc: Phil Muldoon <pmuldoon at redhat dot com>
- Date: Wed, 29 Apr 2015 16:43:48 +0100
- Subject: Re: [PATCH v3 1/9] Code cleanup: Make parts of print_command_1 public
- Authentication-results: sourceware.org; auth=none
- References: <20150411194322 dot 29128 dot 52477 dot stgit at host1 dot jankratochvil dot net> <20150411194333 dot 29128 dot 30245 dot stgit at host1 dot jankratochvil dot net>
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