From: Tom Tromey <tromey@redhat.com>
To: Phil Muldoon <pmuldoon@redhat.com>
Cc: Project Archer <archer@sourceware.org>
Subject: Re: [python] [rfc] Patch for pretty-printers registration API change
Date: Wed, 18 Feb 2009 19:15:00 -0000 [thread overview]
Message-ID: <m3tz6rzhem.fsf@fleche.redhat.com> (raw)
In-Reply-To: <499C1D97.8070803@redhat.com> (Phil Muldoon's message of "Wed\, 18 Feb 2009 14\:39\:19 +0000")
Thanks Phil.
Phil> + function = PyList_GetItem (list, list_index);
Phil> + if (function == NULL)
Phil> + Py_RETURN_NONE;
I think it is not valid to ignore Python errors like this. Instead, I
think you either have to propagate the exception (return NULL) or
clear it. I suspect propagation is the better answer.
Phil> + printer = gdbpy_instantiate_printer (function, value);
Phil> + if (printer && (printer != Py_None))
Still too many parens :)
Also, the same comment holds about error propagation.
Phil> + val_obj = value_to_value_object (value);
This can fail, returning NULL. I think this particular line should
probably be moved out of the TRY_CATCH and after the
GDB_PY_HANDLE_EXCEPTION. Then, it needs an error check.
(Though see below, I think some of this is probably redundant.)
Phil> + Py_DECREF (val_obj);
I'm guessing you'll want Py_XDECREF here.
Phil> value = value_from_contents_and_address (type, valaddr, address);
[...]
Phil> + printer = find_pretty_printer (value);
In this case we know we don't need to copy the value -- we made a new
one and we can just use it directly. So, I think the value_copy logic
should probably be removed from find_pretty_printer and pushed into
the callers that need it. What do you think about that?
Phil> + // Do not instantiate NoneType.
Phil> + if (constructor == Py_None)
Phil> + pretty_printer = Py_None;
Phil> + else
Phil> + pretty_printer = instantiate_pretty_printer (constructor, var->value);
Phil> - if (! constructor)
Phil> + if (! pretty_printer)
Phil> {
Phil> gdbpy_print_stack ();
Phil> error ("Could not evaluate visualizer expression: %s", visualizer);
Phil> }
Phil> - if (constructor == Py_None)
Phil> + if (pretty_printer == Py_None)
Phil> {
Phil> - Py_DECREF (constructor);
Phil> - constructor = NULL;
Phil> + Py_DECREF (pretty_printer);
Phil> + pretty_printer = NULL;
Phil> }
The refcount logic for "None" here is not correct. I think the
simplest fix is to add an incref to the true branch of the first if.
Tom
next prev parent reply other threads:[~2009-02-18 19:15 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-02-17 14:03 Phil Muldoon
2009-02-17 18:52 ` Tom Tromey
2009-02-18 14:40 ` Phil Muldoon
2009-02-18 19:15 ` Tom Tromey [this message]
2009-02-20 15:50 ` Phil Muldoon
2009-02-20 19:38 ` Tom Tromey
2009-02-24 11:30 ` Phil Muldoon
2009-02-24 16:27 ` Tom Tromey
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=m3tz6rzhem.fsf@fleche.redhat.com \
--to=tromey@redhat.com \
--cc=archer@sourceware.org \
--cc=pmuldoon@redhat.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).