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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

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

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/