Re: [RFC 1/8] Language independent bits

[Jan Kratochvil <jan.kratochvil@redhat.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