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

[Andrew Burgess <aburgess@redhat.com>]

Tom Tromey <tom@tromey.com> writes:

>>>>>> "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.

No, I think this is the right amount of paranoid.  I've updated both
these to Tuples, and extended the tests to check these are not
modifiable.

>
> Andrew> +      object->mapped_files = nullptr;
>
> Perhaps this has to be hoisted if the initialization of object->inferior
> does (see earlier review).

Done.

>
> 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.

Fixed the two added in this commit.  I also fixed one from patch #1 too.

>
> 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 ();

Switched to this style.  I was already using this for some of the string
creation stuff, so I'm not sure why I did it differently here.

I posted a V2 series with all these fixes.

Thanks,
Andrew