[13/18] -Wwrite-strings: Wrap PyGetSetDef for construction with string literals

  Unfortunately, PyGetSetDef's 'name' and 'doc' members are 'char *'
instead of 'const char *', meaning that in order to list-initialize
PyGetSetDef arrays using string literals requires writing explicit
'char *' casts.  For example:

    static PyGetSetDef value_object_getset[] = {
   -  { "address", valpy_get_address, NULL, "The address of the value.",
   +  { (char *) "address", valpy_get_address, NULL,
   +    (char *) "The address of the value.",
	NULL },
   -  { "is_optimized_out", valpy_get_is_optimized_out, NULL,
   -    "Boolean telling whether the value is optimized "
   +  { (char *) "is_optimized_out", valpy_get_is_optimized_out, NULL,
   +    (char *) "Boolean telling whether the value is optimized "
	"out (i.e., not available).",
	NULL },
   -  { "type", valpy_get_type, NULL, "Type of the value.", NULL },
   -  { "dynamic_type", valpy_get_dynamic_type, NULL,
   -    "Dynamic type of the value.", NULL },
   -  { "is_lazy", valpy_get_is_lazy, NULL,
   -    "Boolean telling whether the value is lazy (not fetched yet\n\
   +  { (char *) "type", valpy_get_type, NULL,
   +    (char *) "Type of the value.", NULL },
   +  { (char *) "dynamic_type", valpy_get_dynamic_type, NULL,
   +    (char *) "Dynamic type of the value.", NULL },
   +  { (char *) "is_lazy", valpy_get_is_lazy, NULL,
   +    (char *) "Boolean telling whether the value is lazy (not fetched yet\n\
    from the inferior).  A lazy value is fetched when needed, or when\n\
    the \"fetch_lazy()\" method is called.", NULL },
      {NULL}  /* Sentinel */

We have 20 such arrays, and I first wrote a patch that fixed all of
them like that...  It's not pretty, and I can post it if people want
to see it.

One way to make these a bit less ugly would be add a new macro that
hides the casts, like:

  #define GDBPY_GSDEF(NAME, GET, SET, DOC, CLOSURE) \
     { (char *) NAME, GET, SET, (char *) DOC, CLOSURE }

and then use it like:

    static PyGetSetDef value_object_getset[] = {
       GDBPY_GSDEF ("address", valpy_get_address, NULL,
       		    "The address of the value.", NULL),
       GDBPY_GSDEF ("is_optimized_out", valpy_get_is_optimized_out, NULL,
       		    "Boolean telling whether the value is optimized ", NULL),
      {NULL}  /* Sentinel */
    };

But since we have C++11, which gives us constexpr and list
initialization, I thought of a way that requires no changes where the
arrays are initialized:

We add a new type that extends PyGetSetDef (called gdb_PyGetSetDef),
and add constexpr constructors that accept const 'name' and 'doc', and
then list/aggregate initialization simply "calls" these matching
constructors instead.

I put "calls" in quotes, because given "constexpr", it's all done at
compile time, and there's no overhead either in binary size or at run
time.  In fact, we get identical binaries, before/after this change.

I'm a bit undecided whether to change the places that create
PyGetSetDef arrays to explicitly name gdb_PyGetSetDef as type, like:

  -    static PyGetSetDef value_object_getset[] = {
  +    static gdb_PyGetSetDef value_object_getset[] = {

or go the

  #define PyGetSetDef gdb_PyGetSetDef

way as we do for the other Python API fixes.  This commit takes the
latter approach, but I'll change it if people prefer the other way.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* python/python-internal.h (gdb_PyGetSetDef): New type.
	(PyGetSetDef): New define.
---
 gdb/python/python-internal.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
  

On Tuesday, April 04 2017, Pedro Alves wrote:

> Unfortunately, PyGetSetDef's 'name' and 'doc' members are 'char *'
> instead of 'const char *', meaning that in order to list-initialize
> PyGetSetDef arrays using string literals requires writing explicit
> 'char *' casts.  For example:
>
>     static PyGetSetDef value_object_getset[] = {
>    -  { "address", valpy_get_address, NULL, "The address of the value.",
>    +  { (char *) "address", valpy_get_address, NULL,
>    +    (char *) "The address of the value.",
> 	NULL },
>    -  { "is_optimized_out", valpy_get_is_optimized_out, NULL,
>    -    "Boolean telling whether the value is optimized "
>    +  { (char *) "is_optimized_out", valpy_get_is_optimized_out, NULL,
>    +    (char *) "Boolean telling whether the value is optimized "
> 	"out (i.e., not available).",
> 	NULL },
>    -  { "type", valpy_get_type, NULL, "Type of the value.", NULL },
>    -  { "dynamic_type", valpy_get_dynamic_type, NULL,
>    -    "Dynamic type of the value.", NULL },
>    -  { "is_lazy", valpy_get_is_lazy, NULL,
>    -    "Boolean telling whether the value is lazy (not fetched yet\n\
>    +  { (char *) "type", valpy_get_type, NULL,
>    +    (char *) "Type of the value.", NULL },
>    +  { (char *) "dynamic_type", valpy_get_dynamic_type, NULL,
>    +    (char *) "Dynamic type of the value.", NULL },
>    +  { (char *) "is_lazy", valpy_get_is_lazy, NULL,
>    +    (char *) "Boolean telling whether the value is lazy (not fetched yet\n\
>     from the inferior).  A lazy value is fetched when needed, or when\n\
>     the \"fetch_lazy()\" method is called.", NULL },
>       {NULL}  /* Sentinel */
>
> We have 20 such arrays, and I first wrote a patch that fixed all of
> them like that...  It's not pretty, and I can post it if people want
> to see it.
>
> One way to make these a bit less ugly would be add a new macro that
> hides the casts, like:
>
>   #define GDBPY_GSDEF(NAME, GET, SET, DOC, CLOSURE) \
>      { (char *) NAME, GET, SET, (char *) DOC, CLOSURE }
>
> and then use it like:
>
>     static PyGetSetDef value_object_getset[] = {
>        GDBPY_GSDEF ("address", valpy_get_address, NULL,
>        		    "The address of the value.", NULL),
>        GDBPY_GSDEF ("is_optimized_out", valpy_get_is_optimized_out, NULL,
>        		    "Boolean telling whether the value is optimized ", NULL),
>       {NULL}  /* Sentinel */
>     };
>
> But since we have C++11, which gives us constexpr and list
> initialization, I thought of a way that requires no changes where the
> arrays are initialized:
>
> We add a new type that extends PyGetSetDef (called gdb_PyGetSetDef),
> and add constexpr constructors that accept const 'name' and 'doc', and
> then list/aggregate initialization simply "calls" these matching
> constructors instead.
>
> I put "calls" in quotes, because given "constexpr", it's all done at
> compile time, and there's no overhead either in binary size or at run
> time.  In fact, we get identical binaries, before/after this change.
>
> I'm a bit undecided whether to change the places that create
> PyGetSetDef arrays to explicitly name gdb_PyGetSetDef as type, like:
>
>   -    static PyGetSetDef value_object_getset[] = {
>   +    static gdb_PyGetSetDef value_object_getset[] = {
>
> or go the
>
>   #define PyGetSetDef gdb_PyGetSetDef
>
> way as we do for the other Python API fixes.  This commit takes the
> latter approach, but I'll change it if people prefer the other way.

I liked the approach.  As with the other Python patch, I'm more inclined
to use an explicit "gdb_PyGetSetDef" everywhere, just to be clear that
we are using our own version of it.  This can save time when debugging.

Otherwise, the rest is fine by me.

> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
>
> 	* python/python-internal.h (gdb_PyGetSetDef): New type.
> 	(PyGetSetDef): New define.
> ---
>  gdb/python/python-internal.h | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index 55efd75..8fc89cd 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -286,6 +286,38 @@ gdb_PySys_SetPath (const GDB_PYSYS_SETPATH_CHAR *path)
>  
>  #define PySys_SetPath gdb_PySys_SetPath
>  
> +/* Wrap PyGetSetDef to allow convenient construction with string
> +   literals.  Unfortunately, PyGetSetDef's 'name' and 'doc' members
> +   are 'char *' instead of 'const char *', meaning that in order to
> +   list-initialize PyGetSetDef arrays with string literals (and
> +   without the wrapping below) would require writing explicit 'char *'
> +   casts.  Instead, we extend PyGetSetDef and add onstexpr
> +   constructors that accept const 'name' and 'doc', hiding the ugly
> +   casts here in a single place.  */
> +
> +struct gdb_PyGetSetDef : PyGetSetDef
> +{
> +  constexpr gdb_PyGetSetDef (const char *name_, getter get_, setter set_,
> +			     const char *doc_, void *closure_)
> +    : PyGetSetDef {const_cast<char *> (name_), get_, set_,
> +		   const_cast<char *> (doc_), closure_}
> +  {}
> +
> +  /* Alternative constructor that allows omitting the closure in list
> +     initialization.  */
> +  constexpr gdb_PyGetSetDef (const char *name_, getter get_, setter set_,
> +			     const char *doc_)
> +    : gdb_PyGetSetDef {name_, get_, set_, doc_, NULL}
> +  {}
> +
> +  /* Constructor for the sentinel entries.  */
> +  constexpr gdb_PyGetSetDef (std::nullptr_t)
> +    : gdb_PyGetSetDef { NULL, NULL, NULL, NULL, NULL }
> +  {}
> +};
> +
> +#define PyGetSetDef gdb_PyGetSetDef
> +
>  /* In order to be able to parse symtab_and_line_to_sal_object function
>     a real symtab_and_line structure is needed.  */
>  #include "symtab.h"
> -- 
> 2.5.5
  

Hi Pedro

I found a typo ...

On Tue,  4 Apr 2017 18:25:46 +0100
Pedro Alves <palves@redhat.com> wrote:
[...]
> diff --git a/gdb/python/python-internal.h
> b/gdb/python/python-internal.h index 55efd75..8fc89cd 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -286,6 +286,38 @@ gdb_PySys_SetPath (const GDB_PYSYS_SETPATH_CHAR
> *path)
> 
>  #define PySys_SetPath gdb_PySys_SetPath
> 
> +/* Wrap PyGetSetDef to allow convenient construction with string
> +   literals.  Unfortunately, PyGetSetDef's 'name' and 'doc' members
> +   are 'char *' instead of 'const char *', meaning that in order to
> +   list-initialize PyGetSetDef arrays with string literals (and
> +   without the wrapping below) would require writing explicit 'char *'
> +   casts.  Instead, we extend PyGetSetDef and add onstexpr
						     ^^^^^^^^
						... here

> +   constructors that accept const 'name' and 'doc', hiding the ugly
> +   casts here in a single place.  */
> +
> +struct gdb_PyGetSetDef : PyGetSetDef
> +{
> +  constexpr gdb_PyGetSetDef (const char *name_, getter get_, setter
> set_,
> +			     const char *doc_, void *closure_)
> +    : PyGetSetDef {const_cast<char *> (name_), get_, set_,
> +		   const_cast<char *> (doc_), closure_}
> +  {}
> +
> +  /* Alternative constructor that allows omitting the closure in list
> +     initialization.  */
> +  constexpr gdb_PyGetSetDef (const char *name_, getter get_, setter
> set_,
> +			     const char *doc_)
> +    : gdb_PyGetSetDef {name_, get_, set_, doc_, NULL}
> +  {}
> +
> +  /* Constructor for the sentinel entries.  */
> +  constexpr gdb_PyGetSetDef (std::nullptr_t)
> +    : gdb_PyGetSetDef { NULL, NULL, NULL, NULL, NULL }
> +  {}
> +};
> +
> +#define PyGetSetDef gdb_PyGetSetDef
> +
>  /* In order to be able to parse symtab_and_line_to_sal_object
> function a real symtab_and_line structure is needed.  */
>  #include "symtab.h"
  

On 04/05/2017 09:49 AM, Philipp Rudo wrote:
> Hi Pedro
> 
> I found a typo ...

Thanks Philipp.  I fixed it in the version of the patch I sent in
reply to Sergio.

Thanks,
Pedro Alves