RE: [PATCH v3 5/8] python: Create Python bindings for record history.

["Metzger, Markus T" <markus.t.metzger@intel.com>]

> -----Original Message-----
> From: Wiederhake, Tim
> Sent: Monday, November 21, 2016 4:49 PM
> To: gdb-patches@sourceware.org
> Cc: palves@redhat.com; Metzger, Markus T <markus.t.metzger@intel.com>
> Subject: [PATCH v3 5/8] python: Create Python bindings for record history.
> 
> This patch adds three new functions to the gdb module in Python:
> 	- start_recording
> 	- stop_recording
> 	- current_recording
> start_recording and current_recording return an object of the new type
> gdb.Record, which can be used to access the recorded data.
> 
> 2016-11-21  Tim Wiederhake  <tim.wiederhake@intel.com>
> 
> gdb/ChangeLog
> 
> 	* Makefile.in (SUBDIR_PYTHON_OBS): Add python/py-record.o.
> 	(SUBDIR_PYTHON_SRCS): Add python/py-record.c.
> 	* python/py-record.c: New file.
> 	* python/py-record.h: New file.
> 	* python/python-internal.h (gdbpy_start_recording,
> 	gdbpy_current_recording, gdpy_stop_recording,
> 	gdbpy_initialize_record): New export.
> 	* python/python.c (_initialize_python): Add gdbpy_initialize_record.
> 	(python_GdbMethods): Add gdbpy_start_recording,
> 	gdbpy_current_recording and gdbpy_stop_recording.
> 	* target-debug.h (target_debug_print_struct_record_python_interface):
> 	New define.
> 	* target-delegates.c: Regenerate.
> 	* target.c (target_record_python_interface): New function.
> 	* target.h: Added struct record_python_interface forward declaration.
> 	Export target_record_python_interface.
> 	(struct target_ops): Add to_record_python_interface function.



> +/* Python Record object.  */
> +
> +typedef struct
> +{
> +  PyObject_HEAD
> +
> +  struct record_python_interface interface;

You may want to document every field.


> +static PyObject *
> +recpy_method (PyObject *self, void* closure)
> +{
> +  recpy_record_object *obj = (recpy_record_object *) self;
> +
> +  if (obj->interface.method == NULL)
> +    return PyString_FromString (_("unknown"));

Should this return None, instead?

> +
> +  return PyString_FromString (obj->interface.method);
> +}
> +
> +/* Implementation of record.format.  */
> +
> +static PyObject *
> +recpy_format (PyObject *self, void* closure)
> +{
> +  recpy_record_object *obj = (recpy_record_object *) self;
> +
> +  if (obj->interface.format == NULL)
> +    return PyString_FromString (_("unknown"));

Same here.


> +/* Implementation of gdb.current_recording (self) -> gdb.Record.  */
> +
> +PyObject *
> +gdbpy_current_recording (PyObject *self, PyObject *args)
> +{
> +  recpy_record_object* obj;
> +
> +  obj = PyObject_New (recpy_record_object, &recpy_record_type);
> +  if (obj == NULL)
> +    return NULL;
> +
> +  memset (&obj->interface, 0, sizeof (struct record_python_interface));

How about "sizeof (obj->interface)" instead?



> +/* Implementation of gdb.stop_recording (self) -> None.  */
> +
> +PyObject *
> +gdbpy_stop_recording (PyObject *self, PyObject *args)
> +{
> +  TRY
> +    {
> +      execute_command_to_string ("record stop", 0);
> +      Py_RETURN_NONE;
> +    }
> +  CATCH (except, RETURN_MASK_ALL)
> +    {
> +      gdbpy_convert_exception (except);
> +    }
> +  END_CATCH
> +
> +  return NULL;

We shouldn't really get here, should we? 


Thanks,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928