Re: [PATCH 1/2] Add attributes and methods to gdb.Inferior

[Tom Tromey <tromey@adacore.com>]

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

>> +  const char *name;
>> +  if (!PyArg_ParseTuple (args, "s", &name))

Andrew> I'd really prefer gdb_PyArg_ParseTupleAndKeywords be used here too.  I
Andrew> know there's only a single argument, but I think it would be nice if our
Andrew> API was consistent in always accepting keywords.

Ok, I did this.

gdb doesn't generally do this for single-argument functions, but it's
also relatively harmless to do so ("relatively" since it does mean the
name is ABI).

>> +  if (value == nullptr)
>> +    {
>> +      PyErr_SetString (PyExc_TypeError, _("Cannot delete 'arguments' attribute."));

Andrew> Line length?

Fixed.

>> +      return -1;
>> +    }
>> +
>> +  if (PyUnicode_Check (value))

Andrew> I prefer gdbpy_is_string, which is used more.  Though
Andrew> PyUnicode_Check is present in a few places.

I made this change, but it seems to me that gdbpy_is_string was probably
there for Python 2/3 compatibility.  Now that it's just a simple wrapper
for PyUnicode_Check, it seems like it would be more "Pythonically
idiomatic" to just remove the wrapper.

>> +      /* This is unfortunate but the implementation of main_name can
>> +	 reach into memory, among other things.  */
>> +      scoped_restore_current_inferior restore_inferior;
>> +      set_current_inferior (inf->inferior);
>> +
>> +      scoped_restore_current_program_space restore_current_progspace;
>> +      set_current_program_space (inf->inferior->pspace);

Andrew> I guess switch_to_inferior_no_thread would be overkill here, if all
Andrew> we're doing is accessing inferior memory?  Though you do tease us with
Andrew> "among other things", so hopefully nothing is using the incorrect
Andrew> thread....

I removed the "among other things" bit.  I think the thread shouldn't
really matter.

The only offender here is Ada and there's a bug open about how it would
be better to get this info from the executable instead.

>> +
>> +      name = main_name ();
>> +    }
>> +  catch (const gdb_exception &except)
>> +    {
>> +      /* We can just ignore this.  */
>> +      name = nullptr;

Andrew> Is this assignment necessary, given name is initialised to nullptr?

Apparently not, I removed it in v2.

Tom