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

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

On Wednesday, April 05 2017, Pedro Alves wrote:

> On 04/04/2017 07:39 PM, Sergio Durigan Junior wrote:
>
>> 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.
>
> OK, here's what that looks like.  Commit log updated to reflect the
> change too.  WDYT?

LGTM.  Thanks!

> From 566fd938f2919985db6a955d769ba619534964e8 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Wed, 5 Apr 2017 13:22:25 +0100
> Subject: [PATCH] -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...
>
> 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.
>
> Unlike the fixes that fix some old Python API to match the API of more
> recent Python, this switches to using explicit "gdb_PyGetSetDef"
> everywhere, just to be clear that we are using our own version of it.
>
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
>
> 	* python/python-internal.h (gdb_PyGetSetDef): New type.
> 	* python/py-block.c (block_object_getset)
> 	(breakpoint_object_getset): Now a gdb_PyGetSetDef array.
> 	* python/py-event.c (event_object_getset)
> 	(finish_breakpoint_object_getset): Likewise.
> 	* python/py-inferior.c (inferior_object_getset): Likewise.
> 	* python/py-infthread.c (thread_object_getset): Likewise.
> 	* python/py-lazy-string.c (lazy_string_object_getset): Likewise.
> 	* python/py-linetable.c (linetable_entry_object_getset): Likewise.
> 	* python/py-objfile.c (objfile_getset): Likewise.
> 	* python/py-progspace.c (pspace_getset): Likewise.
> 	* python/py-record-btrace.c (btpy_insn_getset, btpy_call_getset):
> 	Likewise.
> 	* python/py-record.c (recpy_record_getset): Likewise.
> 	* python/py-symbol.c (symbol_object_getset): Likewise.
> 	* python/py-symtab.c (symtab_object_getset, sal_object_getset):
> 	Likewise.
> 	* python/py-type.c (type_object_getset, field_object_getset):
> 	Likewise.
> 	* python/py-value.c (value_object_getset): Likewise.
> ---
>  gdb/python/py-block.c            |  2 +-
>  gdb/python/py-breakpoint.c       |  2 +-
>  gdb/python/py-event.c            |  2 +-
>  gdb/python/py-finishbreakpoint.c |  2 +-
>  gdb/python/py-inferior.c         |  2 +-
>  gdb/python/py-infthread.c        |  2 +-
>  gdb/python/py-lazy-string.c      |  2 +-
>  gdb/python/py-linetable.c        |  2 +-
>  gdb/python/py-objfile.c          |  2 +-
>  gdb/python/py-progspace.c        |  2 +-
>  gdb/python/py-record-btrace.c    |  4 ++--
>  gdb/python/py-record.c           |  2 +-
>  gdb/python/py-symbol.c           |  2 +-
>  gdb/python/py-symtab.c           |  4 ++--
>  gdb/python/py-type.c             |  4 ++--
>  gdb/python/py-value.c            |  2 +-
>  gdb/python/python-internal.h     | 30 ++++++++++++++++++++++++++++++
>  17 files changed, 49 insertions(+), 19 deletions(-)
>
> diff --git a/gdb/python/py-block.c b/gdb/python/py-block.c
> index f477d4a..840c842 100644
> --- a/gdb/python/py-block.c
> +++ b/gdb/python/py-block.c
> @@ -461,7 +461,7 @@ Return true if this block is valid, false if not." },
>    {NULL}  /* Sentinel */
>  };
>  
> -static PyGetSetDef block_object_getset[] = {
> +static gdb_PyGetSetDef block_object_getset[] = {
>    { "start", blpy_get_start, NULL, "Start address of the block.", NULL },
>    { "end", blpy_get_end, NULL, "End address of the block.", NULL },
>    { "function", blpy_get_function, NULL,
> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index 724a7ed..34f46fb 100644
> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -1048,7 +1048,7 @@ local_setattro (PyObject *self, PyObject *name, PyObject *v)
>    return PyObject_GenericSetAttr ((PyObject *)self, name, v);
>  }
>  
> -static PyGetSetDef breakpoint_object_getset[] = {
> +static gdb_PyGetSetDef breakpoint_object_getset[] = {
>    { "enabled", bppy_get_enabled, bppy_set_enabled,
>      "Boolean telling whether the breakpoint is enabled.", NULL },
>    { "silent", bppy_get_silent, bppy_set_silent,
> diff --git a/gdb/python/py-event.c b/gdb/python/py-event.c
> index 127dcc7..dc1d73e 100644
> --- a/gdb/python/py-event.c
> +++ b/gdb/python/py-event.c
> @@ -114,7 +114,7 @@ evpy_emit_event (PyObject *event,
>    return 0;
>  }
>  
> -static PyGetSetDef event_object_getset[] =
> +static gdb_PyGetSetDef event_object_getset[] =
>  {
>    { "__dict__", gdb_py_generic_dict, NULL,
>      "The __dict__ for this event.", &event_object_type },
> diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
> index 106fe34..76189b8 100644
> --- a/gdb/python/py-finishbreakpoint.c
> +++ b/gdb/python/py-finishbreakpoint.c
> @@ -426,7 +426,7 @@ gdbpy_initialize_finishbreakpoints (void)
>    return 0;
>  }
>  
> -static PyGetSetDef finish_breakpoint_object_getset[] = {
> +static gdb_PyGetSetDef finish_breakpoint_object_getset[] = {
>    { "return_value", bpfinishpy_get_returnvalue, NULL,
>    "gdb.Value object representing the return value, if any. \
>  None otherwise.", NULL },
> diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
> index 46a0aad..77fc543 100644
> --- a/gdb/python/py-inferior.c
> +++ b/gdb/python/py-inferior.c
> @@ -827,7 +827,7 @@ gdbpy_initialize_inferior (void)
>  				 &membuf_object_type);
>  }
>  
> -static PyGetSetDef inferior_object_getset[] =
> +static gdb_PyGetSetDef inferior_object_getset[] =
>  {
>    { "num", infpy_get_num, NULL, "ID of inferior, as assigned by GDB.", NULL },
>    { "pid", infpy_get_pid, NULL, "PID of inferior, as assigned by the OS.",
> diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c
> index 5482bf9..626c15c 100644
> --- a/gdb/python/py-infthread.c
> +++ b/gdb/python/py-infthread.c
> @@ -304,7 +304,7 @@ gdbpy_initialize_thread (void)
>  				 (PyObject *) &thread_object_type);
>  }
>  
> -static PyGetSetDef thread_object_getset[] =
> +static gdb_PyGetSetDef thread_object_getset[] =
>  {
>    { "name", thpy_get_name, thpy_set_name,
>      "The name of the thread, as set by the user or the OS.", NULL },
> diff --git a/gdb/python/py-lazy-string.c b/gdb/python/py-lazy-string.c
> index ab3f411..1999033 100644
> --- a/gdb/python/py-lazy-string.c
> +++ b/gdb/python/py-lazy-string.c
> @@ -300,7 +300,7 @@ static PyMethodDef lazy_string_object_methods[] = {
>  };
>  
>  
> -static PyGetSetDef lazy_string_object_getset[] = {
> +static gdb_PyGetSetDef lazy_string_object_getset[] = {
>    { "address", stpy_get_address, NULL, "Address of the string.", NULL },
>    { "encoding", stpy_get_encoding, NULL, "Encoding of the string.", NULL },
>    { "length", stpy_get_length, NULL, "Length of the string.", NULL },
> diff --git a/gdb/python/py-linetable.c b/gdb/python/py-linetable.c
> index a5e57b0..8d17aab 100644
> --- a/gdb/python/py-linetable.c
> +++ b/gdb/python/py-linetable.c
> @@ -550,7 +550,7 @@ PyTypeObject ltpy_iterator_object_type = {
>  };
>  
>  
> -static PyGetSetDef linetable_entry_object_getset[] = {
> +static gdb_PyGetSetDef linetable_entry_object_getset[] = {
>    { "line", ltpy_entry_get_line, NULL,
>      "The line number in the source file.", NULL },
>    { "pc", ltpy_entry_get_pc, NULL,
> diff --git a/gdb/python/py-objfile.c b/gdb/python/py-objfile.c
> index 105d88a..6a47c17 100644
> --- a/gdb/python/py-objfile.c
> +++ b/gdb/python/py-objfile.c
> @@ -670,7 +670,7 @@ Add FILE_NAME to the list of files containing debug info for the objfile." },
>    { NULL }
>  };
>  
> -static PyGetSetDef objfile_getset[] =
> +static gdb_PyGetSetDef objfile_getset[] =
>  {
>    { "__dict__", gdb_py_generic_dict, NULL,
>      "The __dict__ for this objfile.", &objfile_object_type },
> diff --git a/gdb/python/py-progspace.c b/gdb/python/py-progspace.c
> index 1e06a75..edabba4 100644
> --- a/gdb/python/py-progspace.c
> +++ b/gdb/python/py-progspace.c
> @@ -378,7 +378,7 @@ gdbpy_initialize_pspace (void)
>  
>  
>  
> -static PyGetSetDef pspace_getset[] =
> +static gdb_PyGetSetDef pspace_getset[] =
>  {
>    { "__dict__", gdb_py_generic_dict, NULL,
>      "The __dict__ for this progspace.", &pspace_object_type },
> diff --git a/gdb/python/py-record-btrace.c b/gdb/python/py-record-btrace.c
> index c816332..6d08121 100644
> --- a/gdb/python/py-record-btrace.c
> +++ b/gdb/python/py-record-btrace.c
> @@ -903,7 +903,7 @@ recpy_bt_goto (PyObject *self, PyObject *args)
>  
>  /* BtraceInstruction members.  */
>  
> -struct PyGetSetDef btpy_insn_getset[] =
> +struct gdb_PyGetSetDef btpy_insn_getset[] =
>  {
>    { "number", btpy_number, NULL, "instruction number", NULL},
>    { "error", btpy_insn_error, NULL, "error number for gaps", NULL},
> @@ -920,7 +920,7 @@ executed speculatively", NULL},
>  
>  /* BtraceFunctionCall members.  */
>  
> -static PyGetSetDef btpy_call_getset[] =
> +static gdb_PyGetSetDef btpy_call_getset[] =
>  {
>    { "number", btpy_number, NULL, "function call number", NULL},
>    { "level", btpy_call_level, NULL, "call stack level", NULL},
> diff --git a/gdb/python/py-record.c b/gdb/python/py-record.c
> index 72922a4..60c0a7c 100644
> --- a/gdb/python/py-record.c
> +++ b/gdb/python/py-record.c
> @@ -175,7 +175,7 @@ Rewind to given location."},
>  
>  /* Record member list.  */
>  
> -static PyGetSetDef recpy_record_getset[] = {
> +static gdb_PyGetSetDef recpy_record_getset[] = {
>    { "ptid", recpy_ptid, NULL, "Current thread.", NULL },
>    { "method", recpy_method, NULL, "Current recording method.", NULL },
>    { "format", recpy_format, NULL, "Current recording format.", NULL },
> diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
> index b71cfb4..05b002f 100644
> --- a/gdb/python/py-symbol.c
> +++ b/gdb/python/py-symbol.c
> @@ -560,7 +560,7 @@ gdbpy_initialize_symbols (void)
>  
>  
>  
> -static PyGetSetDef symbol_object_getset[] = {
> +static gdb_PyGetSetDef symbol_object_getset[] = {
>    { "type", sympy_get_type, NULL,
>      "Type of the symbol.", NULL },
>    { "symtab", sympy_get_symtab, NULL,
> diff --git a/gdb/python/py-symtab.c b/gdb/python/py-symtab.c
> index 09cab22..53b160e 100644
> --- a/gdb/python/py-symtab.c
> +++ b/gdb/python/py-symtab.c
> @@ -544,7 +544,7 @@ gdbpy_initialize_symtabs (void)
>  
>  
>  
> -static PyGetSetDef symtab_object_getset[] = {
> +static gdb_PyGetSetDef symtab_object_getset[] = {
>    { "filename", stpy_get_filename, NULL,
>      "The symbol table's source filename.", NULL },
>    { "objfile", stpy_get_objfile, NULL, "The symtab's objfile.",
> @@ -606,7 +606,7 @@ PyTypeObject symtab_object_type = {
>    symtab_object_getset		  /*tp_getset */
>  };
>  
> -static PyGetSetDef sal_object_getset[] = {
> +static gdb_PyGetSetDef sal_object_getset[] = {
>    { "symtab", salpy_get_symtab, NULL, "Symtab object.", NULL },
>    { "pc", salpy_get_pc, NULL, "Return the symtab_and_line's pc.", NULL },
>    { "last", salpy_get_last, NULL,
> diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
> index f071006..12b6310 100644
> --- a/gdb/python/py-type.c
> +++ b/gdb/python/py-type.c
> @@ -1413,7 +1413,7 @@ gdbpy_initialize_types (void)
>  
>  
>  
> -static PyGetSetDef type_object_getset[] =
> +static gdb_PyGetSetDef type_object_getset[] =
>  {
>    { "code", typy_get_code, NULL,
>      "The code for this type.", NULL },
> @@ -1587,7 +1587,7 @@ PyTypeObject type_object_type =
>    0,				  /* tp_new */
>  };
>  
> -static PyGetSetDef field_object_getset[] =
> +static gdb_PyGetSetDef field_object_getset[] =
>  {
>    { "__dict__", gdb_py_generic_dict, NULL,
>      "The __dict__ for this field.", &field_object_type },
> diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
> index bb42e8b..9c0470f 100644
> --- a/gdb/python/py-value.c
> +++ b/gdb/python/py-value.c
> @@ -1767,7 +1767,7 @@ gdbpy_initialize_values (void)
>  
>  
>  
> -static PyGetSetDef value_object_getset[] = {
> +static gdb_PyGetSetDef value_object_getset[] = {
>    { "address", valpy_get_address, NULL, "The address of the value.",
>      NULL },
>    { "is_optimized_out", valpy_get_is_optimized_out, NULL,
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index 55efd75..d4ed170 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -286,6 +286,36 @@ 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 constexpr
> +   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 }
> +  {}
> +};
> +
>  /* 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/