RE: [PATCH v5 8/9] python: Add tests for record Python bindings

["Wiederhake, Tim" <tim dot wiederhake at intel dot com>]

> Hi. A few issues noted below.

Hi Doug, thanks for reviewing. See below for comments.

> > +/* Implementation of BtraceInstruction.sal [gdb.Symtab_and_line].
> > +   Return the symbol associated with this instruction.  */
> s/symbol/SAL/

Changed locally.

> > +  return PyMemoryView_FromObject (object); }
> I looked at py-inferior.c:infpy_read_memory.
> Not sure how much we want to be consistent here, do we want to wrap the
> result here in a "membuf"?
> I can believe it's overkill for the task at hand, just thought I'd mention it.
> I'm assuming there are no python2-vs-3 issues here.

To use "membuf", I'd have to factor it out from py-inferior.c and adapt it to support read-only buffers.
Memoryview is part of the Python standard library in Python 2.7 as well as Python 3 and seemed like the right way to go.

> > +  strfile = mem_fileopen ();
> > +  gdb_print_insn (target_gdbarch (), insn->pc, strfile, NULL);

> I don't know if any of the btrace_* functions an throw a gdb error, but
> gdb_print_insn can.
> So at least that needs to be wrapped in a TRY/CATCH.
> 
> Suggest looking at the c++ support that archpy_disassemble uses.
> mem_fileopen is gone now.

Changed locally.

> > +static PyObject *
> > +btpy_list_count (PyObject *self, PyObject *value) {
> > +  const LONGEST index = btpy_list_position (self, value);
> > +
> > +  if (index < 0)
> > +    return PyInt_FromLong (0);
> > +  else
> > +    return PyInt_FromLong (1);
> 
> It's not obvious from a simple reading why "count" is 0 or 1.
> Suggest adding a comment.

Changed locally.

> > +  TRY
> > +    {
> > +      struct btrace_insn_iterator iter;
> > +      char buffer[256] = { '\0' };
> Buffer is unused.

Changed locally.

> > +struct PyGetSetDef btpy_insn_getset[] = {
> "{" goes on line by itself

This is handled very inconsistently. For Example, py-arch.c and py-frame.c (and others) have the opening curly brace on the same line whereas py-type.c puts it on the next line. Locally changed as requested.

Tim
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