Re: [RFC] Let "gcore" command accept a suffix argument

[Joel Brobecker <brobecker at adacore dot com>]

> 2010-01-06  Hui Zhu  <teawater@gmail.com>
> 	* printcmd.c (ctype.h): New include.
> 	(eval_command): New function.
> 	(_initialize_printcmd): New command "eval".

This is looking interesting :). A few comments:

  * what is the exact semantics of the eval command? I looked at
    the implementation, and I'm not I understand, or what I understand
    does not necessarily makes sense to me.

    So, in plain English first, what is the eval command expected
    to do, in particular, what parts and how will the command translate
    in the argument that gets passed.

  * implementation-wise:

> +#define CMDSIZE 1024
> +  char cmd[CMDSIZE + 1];

The GNU Coding Standard explicitly recommend against hard-coded
arbitrary limitations like these. In particular, your implementation
seems to just silently truncate the result if the user uses
an argument whose result does not fit in your CMDSIZE.

I think the implementation should allow for the argument to grow
to any size.

> +              if (strlen (eval_begin))
> +                {
> +                  value = parse_and_eval (eval_begin);
> +
> +                  switch (TYPE_CODE (value_type (value)))
> +                    {
> +                      case TYPE_CODE_ARRAY:
> +                        LA_GET_STRING (value, &buffer, &length,
> +                                       &char_type, &la_encoding);
> +                       break;
> +                      case TYPE_CODE_INT:
> +                        buffer = plongest (value_as_long (value));
> +                        length = strlen (buffer);
> +                        break;
> +                      default:
> +                        buffer = eval_begin;
> +                        length = exp - eval_begin;
> +                        break;
> +                    }
> +
> +                  if (length > CMDSIZE - (cmdp - cmd))
> +                    length = CMDSIZE - (cmdp - cmd);
> +                  memcpy (cmdp, buffer, length);
> +                  cmdp += length;

I think you should print "value_print" instead of doing the printing
yourself. Your implementation is missing a lot of other cases that
you need to handle (eg: TYPE_CODE_RANGE, just to mention one).


-- 
Joel