Re: [patch][python] 1/3 Python representation of GDB line tables (Python code)

[Doug Evans <dje at google dot com>]

Phil Muldoon writes:
 > On 23/10/13 21:46, Tom Tromey wrote:
 > >>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
 > > 
 > > Phil> This patch series (three emails) introduces GDB line table representation
 > > Phil> to Python.
 > > 
 > > Phil> This first email contains the GDB and Python code to represent the line
 > > Phil> table data as Python objects.
 > > 
 > > Phil> What do you think?
 > > 
 > > Thanks, Phil.
 > > 
 > > Really just nits here.
 > 
 > Fixed up nits.
 >  
 > > Phil> +  /* Return a frozen set to remove duplicated line numbers in the case
 > > Phil> +     where a source line has more than one instruction and more than one
 > > Phil> +     entry in the line table.  */
 > > Phil> +  fset = PyFrozenSet_New (source_list);
 > > Phil> +  Py_DECREF (source_list);
 > > 
 > > PyFrozenSet_New is new in 2.5.
 > > 
 > > I think it's cheaper, and no more difficult, to just build a set object
 > > from the start rather than make a list and the convert it.  I'm not sure
 > > what to about Python 2.4 though.
 > 
 > All API access to sets is new in 2.5 (sets appeared in the Python
 > default library in 2.3)
 > 
 > I elected here just to use a dictionary to ensure unique line numbers
 > only.
 > 
 > Modified patch follows,
 > 
 > Cheers,
 > 
 > Phil

Hi.  I have some nits too.  Sorry!
Fortunately they're all just nits.

 > diff --git a/gdb/python/py-linetable.c b/gdb/python/py-linetable.c
 > new file mode 100644
 > index 0000000..9cde481
 > --- /dev/null
 > +++ b/gdb/python/py-linetable.c
 > @@ -0,0 +1,621 @@
 > +/* Python interface to line tables.
 > +
 > +   Copyright (C) 2013 Free Software Foundation, Inc.
 > +
 > +   This file is part of GDB.
 > +
 > +   This program is free software; you can redistribute it and/or modify
 > +   it under the terms of the GNU General Public License as published by
 > +   the Free Software Foundation; either version 3 of the License, or
 > +   (at your option) any later version.
 > +
 > +   This program is distributed in the hope that it will be useful,
 > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
 > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 > +   GNU General Public License for more details.
 > +
 > +   You should have received a copy of the GNU General Public License
 > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 > +
 > +#include "defs.h"
 > +#include "python-internal.h"
 > +#include "exceptions.h"
 > +
 > +typedef struct {
 > +  PyObject_HEAD
 > +  /* The line table source line.  */
 > +  gdb_py_longest line;

Can this be an int?

symtab_and_line and linetable_entry use int, thus I'm guessing most/all
of the API in gdb proper uses int.
Using different types looks weird and I'm left with asking "Why?"

I think int will be enough - if not other places will
notice sooner than the Python API will. :-)

 > +  /* The pc associated with the source line.  */
 > +  CORE_ADDR pc;
 > +} linetable_entry_object;
 > [...]
 > +
 > +/* Internal helper function to build a Python Tuple from a GDB Vector.
 > +   A line table entry can have multiple PCs for a given source line.
 > +   Construct a Tuple of all entries for the given source line, LINE
 > +   from the line table VEC.  Construct one line table entry object per
 > +   address.  */
 > +
 > +static PyObject *
 > +build_tuple_from_vector (gdb_py_longest line, VEC (CORE_ADDR) *vec)

Can this function be renamed?
I read this at the call site below and wondered how a general purpose
vector-to-tuple routine was the right solution.
Then I scolled back up and saw this isn't a general purpose routine,
rather it takes a line number and vector of addresses and returns a tuple
of line table entries.

build_line_table_tuple_from_pcs or whatever works for me,
just something that makes it more explicit what it's doing.

 > +/* Implementation of gdb.LineTable.has_pcs (self, line) -> Boolean.
 > +   Returns a Python Boolean indicating whether a source line has any
 > +   line table entries corresponding to it.  */
 > +
 > +static PyObject *
 > +ltpy_has_pcs (PyObject *self, PyObject *args)

Can this function be renamed too?
I read "has_pcs" and think it takes a list of pcs (plural) as an argument.
I now understand how the name is intended to be read here, but
it took a bit.

Since the inputs are a line table and a line number, and the result
is a boolean indicating if the line number is in the table -->

ltpy_has_line or some such?

 > +/* Implementation of gdb.LineTableEntry.pc (self) -> Unsigned Long.
 > +   Returns an unsigned long associated with the PC of the line table
 > +   entry.  */
 > +
 > +static PyObject *
 > +ltpy_entry_get_pcs (PyObject *self, void *closure)

Sorry again.  Can this function be renamed too?
I read "get_pcs" and think plural.

ltpy_entry_get_pc ?