Re: [PATCH 3/3] gdb/python: add Corefile.mapped_files method

[Tom Tromey <tom@tromey.com>]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> Add a new Corefile.mapped_files method which returns a list of
Andrew> gdb.CorefileMappedFile objects.

Andrew> Each gdb.CorefileMappedFile object represents a file that was mapped
Andrew> into the process when the core file was created.

Thanks.

Andrew> +  /* A List of gdb.CorefileMappedFile objects.  This list is only created
Andrew> +     the first time the user calls gdb.Corefile.mapped_files(), the result
Andrew> +     is cached here.  If this pointer is not NULL then this is an owning
Andrew> +     pointer (i.e. this owns a reference to the List).  */
Andrew> +  PyObject *mapped_files;

A Python list is a mutable object, so I think user code could mess
around with this data.

IMO it would be better to make this a tuple instead.

Andrew> +  /* A List of gdb.CorefileMappedFileRegion objects.  */
Andrew> +  PyObject *regions;

Same here.  Though I wonder if that's overly paranoid of me.

Andrew> +      object->mapped_files = nullptr;

Perhaps this has to be hoisted if the initialization of object->inferior
does (see earlier review).

Andrew> +	  gdbpy_ref<corefile_mapped_file_region_object> region_obj
Andrew> +	    ((corefile_mapped_file_region_object *) PyObject_New

I think the cast of the return value of PyObject_New shouldn't be
needed.  PyObject_New is a macro that supplies the cast.

There were a few cases of this.

Andrew> +  gdbpy_ref<> start (gdb_py_object_from_ulongest (obj->start));
Andrew> +  if (start == nullptr)
Andrew> +    return nullptr;
Andrew> +
Andrew> +  return start.release ();

The null check isn't really needed since the release does the same.
I think it's normal to just write

    return gdb_py_object_from_ulongest (obj->start).release ();

There are a few of these.
Though if you prefer the wordier approach that is fine too I suppose.

Tom