This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: simlpe patch implements eval command (with printf-like format and args)
- From: Michael Snyder <msnyder at specifix dot com>
- To: Yakov Lerner <iler dot ml at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 21 Feb 2008 14:11:35 -0800
- Subject: Re: simlpe patch implements eval command (with printf-like format and args)
- References: <f36b08ee0802210120t9646eewf1ae66469883e6f0@mail.gmail.com>
On Thu, 2008-02-21 at 11:20 +0200, Yakov Lerner wrote:
> This simple patch implements eval command with printf-like syntax:
>
> eval "printf-like-format", comma-separated args
>
> The patch is against cvs-checkedout source. Suggestions are welcome.
> Implementation is very simple.
Not bad at all, for a first contribution. But I suggest
you try to limit the scope a little bit and make it still
simpler -- maybe have it accept only strings, floats and ints
to start -- and see if you can't implement it less intrusively
eg. without taking over the function for another command and
introducing new cross-module infrastructure. Then we can
maybe iteratively improve on it.
I'll give you some style and structure feedback on this one:
>
> +/* Grow allocated buffer and appens to it, without causing quadratic
> slowdown */
"append", of course -- and comments must be punctuated (in this case,
period followed by two spaces". Plus indentation -- consult the GNU
coding standard.
> +/* Grow allocated buffer and appens to it, without causing quadratic
> slowdown */
As above.
> +static void
> +eval_command (char *arg, int from_tty)
> +{
> + struct cleanup *old_cleanups;
> + char *str = gdb_own_xasprintf (arg);
> +
> + old_cleanups = make_cleanup (free_current_contents, &str);
> + execute_command (str, from_tty);
> + do_cleanups (old_chain);
> +}
old_chain is undefined. Probably should be "old_cleanups"?
Actually, except for the comment tails, you've done a commendable
job of following the coding standard. I'd like to see an implementation
that's limited enough not to require the "append" function.
Thanks,
Michael