Re: [PATCH v2 4/4] Convert py-tui.c to the "python safety" approach

[Andrew Burgess <aburgess@redhat.com>]

Tom Tromey <tom@tromey.com> writes:

> This patch mostly converts py-tui.c to use the new Python safety code.
>
> In particular:
>
> * All methods of gdb.TuiWindow are now implemented as straightforward
>   methods of gdbpy_tui_window.
>
> * gdb.register_tui_window is converted and simply returns 'void'.

typo: gdb.register_tui_window should be gdbpy_register_tui_window I think.

>
> I converted this particular file because it was relatively
> straightforward, while also demonstrating most of the features of the
> new approach.  For example, explicit result checks aren't needed,
> try/catch can be removed, and the methods are now written in a natural
> style.
>
> Note that more conversion remains to be done here:
>
> * gdbpy_tui_enabled hasn't been converted and still does explicit
>   checks.
>
> * There's one explicit check in gdbpy_tui_window::set_title.  Fully
>   implementing the safety approach means that some low-level things
>   should eventually be converted to throw; but some work has to be
>   deferred until a lot of the work is complete.

I like the new approach, and think it looks like a great improvement
over the existing code.

> ---
>  gdb/python/py-tui.c          | 208 ++++++++++++++++++-------------------------
>  gdb/python/python-internal.h |   5 +-
>  gdb/python/python.c          |   5 +-
>  3 files changed, 92 insertions(+), 126 deletions(-)
>
> diff --git a/gdb/python/py-tui.c b/gdb/python/py-tui.c
> index 111e0f4c9e3..0755d7b92e3 100644
> --- a/gdb/python/py-tui.c
> +++ b/gdb/python/py-tui.c
> @@ -50,7 +50,34 @@ struct gdbpy_tui_window: public PyObject
>    tui_py_window *window;
>  
>    /* Return true if this object is valid.  */
> -  bool is_valid () const;
> +  bool is_valid ();

Dropping 'const' here is unfortunate, and I think is a consequence of
either noargs_method or maybe wrapped_method requiring non-const.

Can we add a 'const' aware overload of noargs_method, or wrapped_method,
or whatever, and allow is_valid to retain the const qualifier?

I think it would be a shame if a consequence of this work is that we end
up dropping the const qualifiers in many places.

If this works, then there might be other places in the wrapper functions
(patch #3) where we could/should add const overloads.

> +
> +  /* Require that this object be valid.  Throws exception if not.  */
> +  void require_valid ()
> +  {
> +    if (!is_valid ())
> +      gdbpy_err_format (PyExc_RuntimeError, _("TUI window is invalid."));
> +  }
> +
> +  /* Erase the TUI window.  */
> +  void erase ();
> +
> +  /* Python function that writes some text to a TUI window.  */
> +  void write (gdbpy_borrowed_ref args, gdbpy_opt_borrowed_ref kw);
> +
> +  /* Return the width of the TUI window.  */
> +  int width ();
> +
> +  /* Return the height of the TUI window.  */
> +  int height ();
> +
> +  /* Return the title of the TUI window.  */
> +  const std::string &title ();
> +
> +  /* Set the title of the TUI window.  */
> +  void set_title (gdbpy_opt_borrowed_ref new_title);
> +
> +  static PyTypeObject *corresponding_object_type;
>  };
>  
>  extern PyTypeObject gdbpy_tui_window_object_type;
> @@ -150,7 +177,7 @@ class tui_py_window : public tui_win_info
>  /* See gdbpy_tui_window declaration above.  */
>  
>  bool
> -gdbpy_tui_window::is_valid () const
> +gdbpy_tui_window::is_valid ()
>  {
>    return window != nullptr && tui_active;
>  }
> @@ -406,173 +433,109 @@ gdbpy_tui_window_maker::operator() (const char *win_name)
>  
>  /* Implement "gdb.register_window_type".  */
>  
> -PyObject *
> -gdbpy_register_tui_window (PyObject *self, PyObject *args, PyObject *kw)
> +void
> +gdbpy_register_tui_window (gdbpy_borrowed_ref args,
> +			   gdbpy_opt_borrowed_ref kw)
>  {
>    static const char *keywords[] = { "name", "constructor", nullptr };
> -
>    const char *name;
>    PyObject *cons_obj;
> +  gdbpy_arg_parse_tuple_and_keywords (args, kw, "sO", keywords,
> +				      &name, &cons_obj);
>  
> -  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "sO", keywords,
> -					&name, &cons_obj))
> -    return nullptr;
> -
> -  try
> -    {
> -      gdbpy_tui_window_maker constr (gdbpy_ref<>::new_reference (cons_obj));
> -      tui_register_window (name, constr);
> -    }
> -  catch (const gdb_exception &except)
> -    {
> -      return gdbpy_handle_gdb_exception (nullptr, except);
> -    }
> -
> -  return py_none ().release ();
> +  gdbpy_tui_window_maker constr (gdbpy_ref<>::new_reference (cons_obj));
> +  tui_register_window (name, constr);
>  }
>  
>  \f
>  
> -/* Require that "Window" be a valid window.  */
> -
> -#define REQUIRE_WINDOW(Window)					\
> -    do {							\
> -      if (!(Window)->is_valid ())				\
> -	return PyErr_Format (PyExc_RuntimeError,		\
> -			     _("TUI window is invalid."));	\
> -    } while (0)
> -
> -/* Require that "Window" be a valid window.  */
> -
> -#define REQUIRE_WINDOW_FOR_SETTER(Window)			\
> -    do {							\
> -      if (!(Window)->is_valid ())				\
> -	{							\
> -	  PyErr_Format (PyExc_RuntimeError,			\
> -			_("TUI window is invalid."));		\
> -	  return -1;						\
> -	}							\
> -    } while (0)
> -
> -/* Python function which checks the validity of a TUI window
> -   object.  */
> -static PyObject *
> -gdbpy_tui_is_valid (PyObject *self, PyObject *args)
> -{
> -  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
> -
> -  if (win->is_valid ())
> -    return py_true ().release ();
> -  return py_false ().release ();
> -}
> -
>  /* Python function that erases the TUI window.  */
> -static PyObject *
> -gdbpy_tui_erase (PyObject *self, PyObject *args)
> +void
> +gdbpy_tui_window::erase ()
>  {
> -  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
> -
> -  REQUIRE_WINDOW (win);
> -
> -  win->window->erase ();
> -
> -  return py_none ().release ();
> +  require_valid ();
> +  window->erase ();
>  }
>  
>  /* Python function that writes some text to a TUI window.  */
> -static PyObject *
> -gdbpy_tui_write (PyObject *self, PyObject *args, PyObject *kw)
> +void
> +gdbpy_tui_window::write (gdbpy_borrowed_ref args, gdbpy_opt_borrowed_ref kw)
>  {
> +  require_valid ();
> +
>    static const char *keywords[] = { "string", "full_window", nullptr };
>  
> -  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
>    const char *text;
>    int full_window = 0;
>  
> -  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s|i", keywords,
> -					&text, &full_window))
> -    return nullptr;
> -
> -  REQUIRE_WINDOW (win);
> +  gdbpy_arg_parse_tuple_and_keywords (args, kw, "s|i", keywords,
> +				      &text, &full_window);
>  
> -  win->window->output (text, full_window);
> -
> -  return py_none ().release ();
> +  window->output (text, full_window);
>  }
>  
> -/* Return the width of the TUI window.  */
> -static PyObject *
> -gdbpy_tui_width (PyObject *self, void *closure)
> +int
> +gdbpy_tui_window::width ()
>  {
> -  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
> -  REQUIRE_WINDOW (win);
> -  gdbpy_ref<> result
> -    = gdb_py_object_from_longest (win->window->viewport_width ());
> -  return result.release ();
> +  require_valid ();
> +  return window->viewport_width ();
>  }
>  
> -/* Return the height of the TUI window.  */
> -static PyObject *
> -gdbpy_tui_height (PyObject *self, void *closure)
> +int
> +gdbpy_tui_window::height ()
>  {
> -  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
> -  REQUIRE_WINDOW (win);
> -  gdbpy_ref<> result
> -    = gdb_py_object_from_longest (win->window->viewport_height ());
> -  return result.release ();
> +  require_valid ();
> +  return window->viewport_height ();
>  }
>  
> -/* Return the title of the TUI window.  */
> -static PyObject *
> -gdbpy_tui_title (PyObject *self, void *closure)
> +const std::string &
> +gdbpy_tui_window::title ()
>  {
> -  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
> -  REQUIRE_WINDOW (win);
> -  return host_string_to_python_string (win->window->title ().c_str ()).release ();
> +  require_valid ();
> +  return window->title ();
>  }
>  
>  /* Set the title of the TUI window.  */
> -static int
> -gdbpy_tui_set_title (PyObject *self, PyObject *newvalue, void *closure)
> +void
> +gdbpy_tui_window::set_title (gdbpy_opt_borrowed_ref new_title)
>  {
> -  gdbpy_tui_window *win = (gdbpy_tui_window *) self;
> +  require_valid ();
>  
> -  REQUIRE_WINDOW_FOR_SETTER (win);
> -
> -  if (newvalue == nullptr)
> -    {
> -      PyErr_Format (PyExc_TypeError, _("Cannot delete \"title\" attribute."));
> -      return -1;
> -    }
> +  if (new_title == nullptr)
> +    gdbpy_err_format (PyExc_TypeError,
> +		      _("Cannot delete \"title\" attribute."));
>  
> +  /* FIXME: safety */

Please can you expand this comment.  I know this is something you're
actively working on, and the plan is that this comment probably
shouldn't exist in the code for too long.  But it might, so we should
assume that it will, and write this accordingly.

For me, and 'FIXME' command should explain (1) what needs fixing, and
(2) why it cannot currently be fixed.  Armed with these two pieces of
information I can, in the future, make an informed decision about
whether the issue has been, or can now, be fixed.

I'll be honest, even with the commit message hint, I don't really
understand what it is that needs fixing here, I guess it's that you'd
like python_string_to_host_string to throw rather than return NULL?  But
the issue is that this function is used in non-safety code, so cannot
(currently) be changed?  So we'd eventually have:

    gdb::unique_xmalloc_ptr<char> value
      = python_string_to_host_string (new_title);
    gdb_assert (value != nullptr);
    ... etc ...


Thanks,
Andrew