This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC 1/8] Language independent bits
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: Sergio Durigan Junior <sergiodj at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Sun, 15 Jan 2012 21:34:20 +0100
- Subject: Re: [RFC 1/8] Language independent bits
- References: <m3k44s7qej.fsf@gmail.com> <m3aa5o7q3k.fsf@gmail.com>
On Sun, 15 Jan 2012 19:55:27 +0100, Sergio Durigan Junior wrote:
> static int
> -unk_lang_parser (void)
> +unk_lang_parser (struct parser_state *ps)
> {
> return 1;
> }
>
> static void
> -unk_lang_error (char *msg)
> +unk_lang_error (struct parser_state *ps, char *msg)
> {
> error (_("Attempted to parse an expression with unknown language"));
> }
Do you use `ps' in these two functions in some later patch? Otherwise I do
not think it should be passed. It can be always passed (sure incl. updating
callers and the callers of the callers) later when needed.
> +static void
> +initialize_expout (struct parser_state *ps, int initial_size,
> + const struct language_defn *lang,
> + struct gdbarch *gdbarch)
> +{
> + ps->expout_size = initial_size;
> + ps->expout_ptr = 0;
> + ps->expout = (struct expression *)
> + xmalloc (sizeof (struct expression)
> + + EXP_ELEM_TO_BYTES (ps->expout_size));
That return type cast is excessive. It had meaning for K&R C where xmalloc
returns PTR but that is no longer supported. There should be done
s/PTR/void */ everywhere and #define PTR should be removed
and #pragma GCC poison -ed.
> + ps->expout->language_defn = lang;
> + ps->expout->gdbarch = gdbarch;
> +}
[...]
> -extern struct expression *expout;
> -extern int expout_size;
> -extern int expout_ptr;
> +#define parse_gdbarch(ps) (ps->expout->gdbarch)
> +#define parse_language(ps) (ps->expout->language_defn)
->
#define parse_gdbarch(ps) ((ps)->expout->gdbarch)
#define parse_language(ps) ((ps)->expout->language_defn)
>
> -#define parse_gdbarch (expout->gdbarch)
> -#define parse_language (expout->language_defn)
> +struct parser_state {
Opening { should be on a new line.
> +
> + /* The expression related to this parser state. */
Indentation should be by 2, not 4 spaces.
> +
> + struct expression *expout;
> +
> + /* The size of the expression above. */
> +
> + int expout_size;
Memory object size does not fit into `int', it should be `size_t'.
> +
> + /* The number of elements already in the expression. This is used
> + to know where to put new elements. */
> +
> + int expout_ptr;
Likewise.
> +};
[...]
> +/* Reallocate the `expout' pointer inside PS so that it can accommodate
> + at least LENELT expression elements. This function does nothing if
> + there is enough room for the elements. */
> +
> +extern void increase_expout_size (struct parser_state *ps, int lenelt);
Memory object size does not fit into `int', it should be `size_t'.
Sorry this is not review/approval, just comments.
Thanks,
Jan