Re: [PATCH 14/18] -Wwrite-strings: Add a PyArg_ParseTupleAndKeywords "const char *" overload

Sergio Durigan Junior
2017-04-04T18:37:53Z

On Tuesday, April 04 2017, Pedro Alves wrote:

> -Wwrite-strings flags code like:
>
> static char *keywords[] = {"command", "from_tty", "to_string", NULL };
>
> as needing "(char *)" casts, because string literals are "const char []".
>
> We can get rid of the casts by changing the array like this:
>
> - static char *keywords[] = {"command", "from_tty", "to_string", NULL };
> + static const char *keywords[] = {"command", "from_tty", "to_string", NULL };
>
> However, passing the array to PyArg_ParseTupleAndKeywords no longer
> works OOTB, because PyArg_ParseTupleAndKeywords expects a "char **":
>
> PyArg_ParseTupleAndKeywords(PyObject *args, PyObject *kw,
> const char *format,
> char *keywords[], ...);
>
> and "const char **" is not implicitly convertible to "char **". C++
> is more tolerant that C here WRT aliasing, and a const_cast<char **>
> would be fine.
>
> However, to avoid having all callers add the 'const_cast<char **>'
> themselves, this commit define a PyArg_ParseTupleAndKeywords overload
> here with a corresponding 'keywords' parameter type that does the cast
> in a single place.
>
> As in the PyGetSetDef patch, I'd be fine with naming this
> gdb_PyArg_ParseTupleAndKeywords instead, if people would find having
> our own overload confusing.

As much as I like the overloading approach in general, I prefer to see
our own overloads marked with the "gdb_", just to make things clearer.

> BTW, this API issue was discussed years ago in the python-dev list:
>
> https://mail.python.org/pipermail/python-dev/2006-February/060689.html
>
> ... and an overload for C++ was suggested in there somewhere.
> Unfortunately, it was never added.

Interesting. I wonder if there's an actual bug/issue opened on their
bugzilla about this.

Hm, after a quick search I didn't find anything. Maybe it's worth
creating one.

The rest is fine by me. Thanks!

> gdb/ChangeLog:
> yyyy-mm-dd Pedro Alves <palves@redhat.com>
>
> * python/py-arch.c (archpy_disassemble): Constify 'keywords'
> array.
> * python/py-cmd.c (cmdpy_init): Constify 'keywords' array.
> * python/py-finishbreakpoint.c (bpfinishpy_init): Constify
> 'keywords' array.
> * python/py-inferior.c (infpy_read_memory, infpy_write_memory)
> (infpy_search_memory): Constify 'keywords' array.
> * python/py-objfile.c (objfpy_add_separate_debug_file)
> (gdbpy_lookup_objfile): Constify 'keywords' array.
> * python/py-symbol.c (gdbpy_lookup_symbol)
> (gdbpy_lookup_global_symbol): Constify 'keywords' array.
> * python/py-type.c (gdbpy_lookup_type): Constify 'keywords' array.
> * python/py-value.c (valpy_lazy_string, valpy_string): Constify
> 'keywords' array.
> * python/python.c (execute_gdb_command, gdbpy_write, gdbpy_flush):
> Constify 'keywords' array.
> * python/python-internal.h (PyArg_ParseTupleAndKeywords): New
> function overload.
> ---
> gdb/python/py-arch.c | 2 +-
> gdb/python/py-breakpoint.c | 4 ++--
> gdb/python/py-cmd.c | 6 +++---
> gdb/python/py-finishbreakpoint.c | 2 +-
> gdb/python/py-inferior.c | 6 +++---
> gdb/python/py-objfile.c | 4 ++--
> gdb/python/py-symbol.c | 4 ++--
> gdb/python/py-type.c | 2 +-
> gdb/python/py-value.c | 4 ++--
> gdb/python/python-internal.h | 25 +++++++++++++++++++++++++
> gdb/python/python.c | 6 +++---
> 11 files changed, 45 insertions(+), 20 deletions(-)
>
> diff --git a/gdb/python/py-arch.c b/gdb/python/py-arch.c
> index 71d2989..48fae32 100644
> --- a/gdb/python/py-arch.c
> +++ b/gdb/python/py-arch.c
> @@ -116,7 +116,7 @@ archpy_name (PyObject *self, PyObject *args)
> static PyObject *
> archpy_disassemble (PyObject *self, PyObject *args, PyObject *kw)
> {
> - static char *keywords[] = { "start_pc", "end_pc", "count", NULL };
> + static const char *keywords[] = { "start_pc", "end_pc", "count", NULL };
> CORE_ADDR start, end = 0;
> CORE_ADDR pc;
> gdb_py_ulongest start_temp;
> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index 724a7ed..2043d42 100644
> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -637,8 +637,8 @@ bppy_get_ignore_count (PyObject *self, void *closure)
> static int
> bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
> {
> - static char *keywords[] = { "spec", "type", "wp_class", "internal",
> - "temporary", NULL };
> + static const char *keywords[] = { "spec", "type", "wp_class", "internal",
> + "temporary", NULL };
> const char *spec;
> int type = bp_breakpoint;
> int access_type = hw_write;
> diff --git a/gdb/python/py-cmd.c b/gdb/python/py-cmd.c
> index 3aaf7f9..8f61d9c 100644
> --- a/gdb/python/py-cmd.c
> +++ b/gdb/python/py-cmd.c
> @@ -490,8 +490,8 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
> char *docstring = NULL;
> struct cmd_list_element **cmd_list;
> char *cmd_name, *pfx_name;
> - static char *keywords[] = { "name", "command_class", "completer_class",
> - "prefix", NULL };
> + static const char *keywords[] = { "name", "command_class", "completer_class",
> + "prefix", NULL };
> PyObject *is_prefix = NULL;
> int cmp;
>
> @@ -506,7 +506,7 @@ cmdpy_init (PyObject *self, PyObject *args, PyObject *kw)
>
> if (! PyArg_ParseTupleAndKeywords (args, kw, "si|iO",
> keywords, &name, &cmdtype,
> - &completetype, &is_prefix))
> + &completetype, &is_prefix))
> return -1;
>
> if (cmdtype != no_class && cmdtype != class_run
> diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
> index 106fe34..9d43d55 100644
> --- a/gdb/python/py-finishbreakpoint.c
> +++ b/gdb/python/py-finishbreakpoint.c
> @@ -156,7 +156,7 @@ bpfinishpy_post_stop_hook (struct gdbpy_breakpoint_object *bp_obj)
> static int
> bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
> {
> - static char *keywords[] = { "frame", "internal", NULL };
> + static const char *keywords[] = { "frame", "internal", NULL };
> struct finish_breakpoint_object *self_bpfinish =
> (struct finish_breakpoint_object *) self;
> PyObject *frame_obj = NULL;
> diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
> index 46a0aad..8b4782b 100644
> --- a/gdb/python/py-inferior.c
> +++ b/gdb/python/py-inferior.c
> @@ -435,7 +435,7 @@ infpy_read_memory (PyObject *self, PyObject *args, PyObject *kw)
> CORE_ADDR addr, length;
> gdb_byte *buffer = NULL;
> PyObject *addr_obj, *length_obj, *result;
> - static char *keywords[] = { "address", "length", NULL };
> + static const char *keywords[] = { "address", "length", NULL };
>
> if (! PyArg_ParseTupleAndKeywords (args, kw, "OO", keywords,
> &addr_obj, &length_obj))
> @@ -494,7 +494,7 @@ infpy_write_memory (PyObject *self, PyObject *args, PyObject *kw)
> const gdb_byte *buffer;
> CORE_ADDR addr, length;
> PyObject *addr_obj, *length_obj = NULL;
> - static char *keywords[] = { "address", "buffer", "length", NULL };
> + static const char *keywords[] = { "address", "buffer", "length", NULL };
> #ifdef IS_PY3K
> Py_buffer pybuf;
>
> @@ -643,7 +643,7 @@ infpy_search_memory (PyObject *self, PyObject *args, PyObject *kw)
> {
> struct gdb_exception except = exception_none;
> CORE_ADDR start_addr, length;
> - static char *keywords[] = { "address", "length", "pattern", NULL };
> + static const char *keywords[] = { "address", "length", "pattern", NULL };
> PyObject *start_addr_obj, *length_obj;
> Py_ssize_t pattern_size;
> const gdb_byte *buffer;
> diff --git a/gdb/python/py-objfile.c b/gdb/python/py-objfile.c
> index 105d88a..ad1ec4f 100644
> --- a/gdb/python/py-objfile.c
> +++ b/gdb/python/py-objfile.c
> @@ -432,7 +432,7 @@ objfpy_is_valid (PyObject *self, PyObject *args)
> static PyObject *
> objfpy_add_separate_debug_file (PyObject *self, PyObject *args, PyObject *kw)
> {
> - static char *keywords[] = { "file_name", NULL };
> + static const char *keywords[] = { "file_name", NULL };
> objfile_object *obj = (objfile_object *) self;
> const char *file_name;
>
> @@ -559,7 +559,7 @@ objfpy_lookup_objfile_by_build_id (const char *build_id)
> PyObject *
> gdbpy_lookup_objfile (PyObject *self, PyObject *args, PyObject *kw)
> {
> - static char *keywords[] = { "name", "by_build_id", NULL };
> + static const char *keywords[] = { "name", "by_build_id", NULL };
> const char *name;
> PyObject *by_build_id_obj = NULL;
> int by_build_id;
> diff --git a/gdb/python/py-symbol.c b/gdb/python/py-symbol.c
> index b71cfb4..de44c06 100644
> --- a/gdb/python/py-symbol.c
> +++ b/gdb/python/py-symbol.c
> @@ -373,7 +373,7 @@ gdbpy_lookup_symbol (PyObject *self, PyObject *args, PyObject *kw)
> int domain = VAR_DOMAIN;
> struct field_of_this_result is_a_field_of_this;
> const char *name;
> - static char *keywords[] = { "name", "block", "domain", NULL };
> + static const char *keywords[] = { "name", "block", "domain", NULL };
> struct symbol *symbol = NULL;
> PyObject *block_obj = NULL, *sym_obj, *bool_obj;
> const struct block *block = NULL;
> @@ -443,7 +443,7 @@ gdbpy_lookup_global_symbol (PyObject *self, PyObject *args, PyObject *kw)
> {
> int domain = VAR_DOMAIN;
> const char *name;
> - static char *keywords[] = { "name", "domain", NULL };
> + static const char *keywords[] = { "name", "domain", NULL };
> struct symbol *symbol = NULL;
> PyObject *sym_obj;
>
> diff --git a/gdb/python/py-type.c b/gdb/python/py-type.c
> index f071006..4f4ede1 100644
> --- a/gdb/python/py-type.c
> +++ b/gdb/python/py-type.c
> @@ -1347,7 +1347,7 @@ type_object_to_type (PyObject *obj)
> PyObject *
> gdbpy_lookup_type (PyObject *self, PyObject *args, PyObject *kw)
> {
> - static char *keywords[] = { "name", "block", NULL };
> + static const char *keywords[] = { "name", "block", NULL };
> const char *type_name = NULL;
> struct type *type = NULL;
> PyObject *block_obj = NULL;
> diff --git a/gdb/python/py-value.c b/gdb/python/py-value.c
> index bb42e8b..1a82af5 100644
> --- a/gdb/python/py-value.c
> +++ b/gdb/python/py-value.c
> @@ -431,7 +431,7 @@ valpy_lazy_string (PyObject *self, PyObject *args, PyObject *kw)
> gdb_py_longest length = -1;
> struct value *value = ((value_object *) self)->value;
> const char *user_encoding = NULL;
> - static char *keywords[] = { "encoding", "length", NULL };
> + static const char *keywords[] = { "encoding", "length", NULL };
> PyObject *str_obj = NULL;
>
> if (!PyArg_ParseTupleAndKeywords (args, kw, "|s" GDB_PY_LL_ARG, keywords,
> @@ -526,7 +526,7 @@ valpy_string (PyObject *self, PyObject *args, PyObject *kw)
> const char *user_encoding = NULL;
> const char *la_encoding = NULL;
> struct type *char_type;
> - static char *keywords[] = { "encoding", "errors", "length", NULL };
> + static const char *keywords[] = { "encoding", "errors", "length", NULL };
>
> if (!PyArg_ParseTupleAndKeywords (args, kw, "|ssi", keywords,
> &user_encoding, &errors, &length))
> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
> index 8fc89cd..76f7443 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -318,6 +318,31 @@ struct gdb_PyGetSetDef : PyGetSetDef
>
> #define PyGetSetDef gdb_PyGetSetDef
>
> +/* The 'keywords' parameter of PyArg_ParseTupleAndKeywords has type
> + 'char **'. However, string literals are const in C++, and so to
> + avoid casting at every keyword array definition, we'll need to make
> + the keywords array an array of 'const char *'. To avoid having all
> + callers add a 'const_cast<char **>' themselves when passing such an
> + array through 'char **', we define a PyArg_ParseTupleAndKeywords
> + overload here with a corresponding 'keywords' parameter type that
> + does the cast in a single place. */
> +
> +static inline int
> +PyArg_ParseTupleAndKeywords (PyObject *args, PyObject *kw,
> + const char *format, const char **keywords, ...)
> +{
> + va_list ap;
> + int res;
> +
> + va_start (ap, keywords);
> + res = PyArg_VaParseTupleAndKeywords (args, kw, format,
> + const_cast<char **> (keywords),
> + ap);
> + va_end (ap);
> +
> + return res;
> +}
> +
> /* 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"
> diff --git a/gdb/python/python.c b/gdb/python/python.c
> index a7aff53..5a46493 100644
> --- a/gdb/python/python.c
> +++ b/gdb/python/python.c
> @@ -572,7 +572,7 @@ execute_gdb_command (PyObject *self, PyObject *args, PyObject *kw)
> const char *arg;
> PyObject *from_tty_obj = NULL, *to_string_obj = NULL;
> int from_tty, to_string;
> - static char *keywords[] = {"command", "from_tty", "to_string", NULL };
> + static const char *keywords[] = { "command", "from_tty", "to_string", NULL };
>
> if (! PyArg_ParseTupleAndKeywords (args, kw, "s|O!O!", keywords, &arg,
> &PyBool_Type, &from_tty_obj,
> @@ -1047,7 +1047,7 @@ static PyObject *
> gdbpy_write (PyObject *self, PyObject *args, PyObject *kw)
> {
> const char *arg;
> - static char *keywords[] = {"text", "stream", NULL };
> + static const char *keywords[] = { "text", "stream", NULL };
> int stream_type = 0;
>
> if (! PyArg_ParseTupleAndKeywords (args, kw, "s|i", keywords, &arg,
> @@ -1088,7 +1088,7 @@ gdbpy_write (PyObject *self, PyObject *args, PyObject *kw)
> static PyObject *
> gdbpy_flush (PyObject *self, PyObject *args, PyObject *kw)
> {
> - static char *keywords[] = {"stream", NULL };
> + static const char *keywords[] = { "stream", NULL };
> int stream_type = 0;
>
> if (! PyArg_ParseTupleAndKeywords (args, kw, "|i", keywords,
> --
> 2.5.5

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