Re: [PATCH 4/8] Add a C++ wrapper for GCC C plug-in

[Pedro Alves <palves@redhat.com>]

On 05/03/2018 07:41 PM, Keith Seitz wrote:
> This patch introduces a new class which wraps the GCC C compile plug-in.
> It is a little unfortunate that this all happened in between the time that
> GCC moved to C++ and GDB moved to C++, leaving us with an ABI promise to
> support a C-like interface.  

It's easier to keep an ABI promise in C anyway, so it's not uncommon
for C++ projects to expose a C plugin interface and then provide wrappers
for multiple languages.  That's how the GCC JIT interface is exported
for example.

> The hope is to isolate GDB from some of this
> should it change in the future.

Not so sure it would be a good idea to change it, IMHO.

> 
> Broadly, what this does is replace calls like:
> 
>   C_CTX (context)->c_ops->operation (C_CTX (context), ...);
> 
> with calls that now look like:
> 
>   context->c_plugin->operation (...);

This LGTM.

I do wonder whether c_plugin needs to be a pointer though?

Couldn't it be an object directly?  Like:

  context->c_plugin.operation (...);

I see that the next patch switches this to a method named
plugin(), and then I wonder if that method can return a reference
instead of a pointer (because IIUC, you can never have a
NULL plugin).

TBC, even if you agree, I don't think it's worth it to go
over the pain of to redoing the series because of this.
I'd be totally fine with changing it on top of the series.

> @@ -206,22 +197,20 @@ convert_enum (struct compile_c_instance *context, struct type *type)
>  {
>    gcc_type int_type, result;
>    int i;
> -  struct gcc_c_context *ctx = C_CTX (context);
> +  const gcc_c_plugin *plugin = context->c_plugin;
>  
> -  int_type = ctx->c_ops->int_type_v0 (ctx,
> -				      TYPE_UNSIGNED (type),
> -				      TYPE_LENGTH (type));
> +  int_type = plugin->int_type_v0 (TYPE_UNSIGNED (type),
> +					    TYPE_LENGTH (type));

Alignment looks odd here, but maybe it's just in the email client.

Thanks,
Pedro Alves