Re: [RFC 1/8] Language independent bits

Jan Kratochvil
2012-01-15T20:34:46Z

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