[v3,1/9] Code cleanup: Make parts of print_command_1 public

  The later 'compile print' command should share its behavior with the existing
'print' command.  Make the needed existing parts of print_command_1 public.

gdb/ChangeLog
2015-03-26  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* printcmd.c (print_command_parse_format, print_value): New functions
	from ...
	(print_command_1): ... here.  Call them.
	* valprint.h (struct format_data, print_command_parse_format)
	(print_value): New declarations.
---
 0 files changed
  

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
  

On Wed, 29 Apr 2015 17:43:48 +0200, Pedro Alves wrote:
> 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.

Yes, you are right it gets leaked in the end.


> I think this all ends up simpler if it follows the pattern that
> the current code already follows.

It was reworked because struct format_data * becomes a part of struct
do_module_cleanup which can be processed independently in the future.
So the forgotten xfree was inteded+forgotten to be in do_module_cleanup.

OTOH this COMPILE_I_PRINT_ADDRESS_SCOPE and COMPILE_I_PRINT_VALUE_SCOPE
processing in do_module_cleanup is suppressed if the caller no longer exists
(the 'compile print' command already finished).  That means that you are right
do_module_cleanup can depend on a local variable of the compile_print_command
caller.

I have to mention with sub-classing and C++ automatic memory management there
would be nothing left to solve.


Jan