Re: [PATCH] c-pragma: adding a data field to pragma_handler

[Tom Tromey <tromey@redhat.com>]

>>>>> "Pierre" == Pierre  <p.vittet@laposte.net> writes:

Pierre> I have changed this handler in order to accept a second parameter
Pierre> which is a void *, allowing to give extra datas to the handler. I
Pierre> think this data field might be of general use: we can have condition
Pierre> or data at register time that we want to express in the handler. I
Pierre> guess this is a common way to pass data to an handler function.

I can't approve or reject this patch, but the idea seems reasonable
enough to me.

Pierre> I would like your opinion on this patch! Thanks!

It has a number of formatting issues.

Pierre> +typedef void (*pragma_handler)(struct cpp_reader *, void * );

No space after the final "*".

Pierre> +/* Internally use to keep the data of the handler.  */
Pierre> +struct internal_pragma_handler_d{

Space before the "{".

Pierre> +  pragma_handler handler;
Pierre> +  void * data; 

No space.  Lots of instances of this.

Pierre>  /* A vector of registered pragma callbacks.  */
Pierre> +/*This is never freed as we need it during the whole execution */

Coalesce the two comments.  The comment formatting is wrong, see GNU
standards.

Pierre>        ns_name.space = space;
Pierre>        ns_name.name = name;
Pierre> +      
Pierre>        VEC_safe_push (pragma_ns_name, heap, registered_pp_pragmas, &ns_name);

Gratuitous newline addition.

Pierre> +      ihandler->handler = handler;
Pierre> +      ihandler->data = data;

I didn't see anything that initialized ihandler.

Pierre> +      VEC_safe_push (internal_pragma_handler, heap, registered_pragmas,
Pierre> +                                                    &ihandler);

I think you wanted just `internal_pragma_handler ihandler', no "*", for
the definition.

Pierre> +c_register_pragma (const char *space, const char *name, pragma_handler handler, 
Pierre> +                   void * data)

There are lots of calls to this that you did not update.
Do a recursive grep to see.

One way to avoid a massive change is to add a new "overload" that passes
in the data to c_register_pragma_1; and then change the "legacy"
functions to pass NULL.

I don't know if that approach is ok (it is typical in gdb...), so if
not, you have to update all callers.

Tom