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

Sergio Durigan Junior
2017-04-04T18:40:03Z

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