public inbox for archer@sourceware.org
 help / color / mirror / Atom feed
From: Richard Ward <richard.j.ward1@googlemail.com>
To: Oguz Kayral <oguzkayral@gmail.com>
Cc: archer@sourceware.org
Subject: Re: [RFC][0/5] Python event handling
Date: Thu, 24 Sep 2009 01:52:00 -0000	[thread overview]
Message-ID: <4ABAD0EA.4020303@googlemail.com> (raw)
In-Reply-To: <36a35d480908230834y46a44e08od4645db444bb5e7a@mail.gmail.com>

Oguz Kayral wrote:
> Hi,
> This patch series adds inferior event handling support to GDB Python
> scripting.

Hi Oguz, I've been looking forward to this functionality, Good stuff!

I have a few comments though, if you don't mind.


Firstly two (fairly  unimportant) things about the function registration.

I think it would be nice if you could give upvalues to the registration 
function (similar to gobject's `connect'), though admittedly it is very 
easy to fake that in python using a class that defines __call__.

Also, I'm not sure whether this would ever be an issue but it seems 
advantageous to have the registration function return a unique object or 
number that could subsequently be used to unregister your callable, 
rather than using the callable object itself.


Next, in python-stopevent.c:
emit_stop_event (struct bpstats *bs, const char *stop_signal)
{
   /*...snip...*/
   for (i = 0; i < PyList_Size (callback_list); i++)
     {
       PyObject_CallObject (PyList_GET_ITEM (callback_list, i), args_tuple);
     }
}

A couple of issues with this. First, PyObject_Callback returns a new 
reference which is the return value of the function (or NULL), so it 
must be handled otherwise it will leak.

Secondly the user code may throw an exception, or the object may not be 
callable. The exception should be cleared, and it would be nice to also 
print the exception (PyErr_Print does both).

Most important here though is the iteration through the list. If the 
user disconnects a callback from inside a callback then the some 
callbacks could be skipped. It could be better to copy the list first, 
but that could still lead to some confusing behaviour, for example a 
callback being called after the user thinks it ought to have been 
disconnected.

Thanks,
Richard


      parent reply	other threads:[~2009-09-24  1:52 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-08-23 15:34 Oguz Kayral
2009-09-21 21:17 ` Tom Tromey
2009-09-24  1:52 ` Richard Ward [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4ABAD0EA.4020303@googlemail.com \
    --to=richard.j.ward1@googlemail.com \
    --cc=archer@sourceware.org \
    --cc=oguzkayral@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).