Patches item #1552731, was opened at 2006-09-05 09:47
Message generated for change (Settings changed) made by rhettinger
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1552731&group_id=5470
Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Core (C code)
Group: Python 2.5
>Status: Closed
Resolution: Accepted
Priority: 7
Submitted By: Raymond Hettinger (rhettinger)
Assigned to: Neal Norwitz (nnorwitz)
Summary: Fix error checks and leaks in setobject.c
Initial Comment:
Check return values for errors.
Fix refcounts on the error returns;
----------------------------------------------------------------------
Comment By: Georg Brandl (gbrandl)
Date: 2006-09-08 01:03
Message:
Logged In: YES
user_id=849994
Sure. Committed revision 51825 for 2.5 branch.
----------------------------------------------------------------------
Comment By: Raymond Hettinger (rhettinger)
Date: 2006-09-07 15:13
Message:
Logged In: YES
user_id=80475
Georg, the buildbot seems to be happy with this patch as
applied to the head, would you please backport it to the
Py2.5 release for me (since I'm away from my svn commit
machine for a while).
Thanks,
Raymond
----------------------------------------------------------------------
Comment By: Neal Norwitz (nnorwitz)
Date: 2006-09-07 02:05
Message:
Logged In: YES
user_id=33168
rev 51798 for 2.6 was checked in.
Georg, true. I was thinking that something like this would
be nicer:
int rv = set_contains_key(so, key);
if (rv == -1 ||
(rv != 0 && set_add_key(result, key) == -1)) {
I figured the transformation would be easy. But now that I
look at the code above, I really don't like that at all.
The only other solution I can think of is to use a goto:
int rv = set_contains_key(so, key);
if (rv == -1)
goto some_error;
if (rv) {
if (set_add_key(result, key) == -1) {
some_error:
I'm not really sure I like that either. So basically no
matter which way the code looks, I'm not gonna be happy. :-)
Oh well.
Raymond, were you planning on backporting this or did you
want someone else to? I volunteer to backport to 2.4. ;-)
----------------------------------------------------------------------
Comment By: Georg Brandl (gbrandl)
Date: 2006-09-06 02:09
Message:
Logged In: YES
user_id=849994
That misses the possibility of set_contains_key's return
value being 0.
----------------------------------------------------------------------
Comment By: Neal Norwitz (nnorwitz)
Date: 2006-09-06 02:04
Message:
Logged In: YES
user_id=33168
This is fine. Though I wonder if hunks like this:
- if (set_contains_key(so, key)) {
+ int rv = set_contains_key(so, key);
+ if (rv == -1) {
+ Py_DECREF(it);
+ Py_DECREF(result);
+ Py_DECREF(key);
+ return NULL;
+ }
+ if (rv) {
if (set_add_key(result, key) == -1) {
would be clearer (to me at least) more like:
if (set_contains_key(so, key) == -1 ||
set_add_key(result, key) == -1) {
That ensures the cleanup is the same. There are several
similar hunks.
----------------------------------------------------------------------
Comment By: Georg Brandl (gbrandl)
Date: 2006-09-06 01:54
Message:
Logged In: YES
user_id=849994
Shouldn't this go into 2.5 final?
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1552731&group_id=5470
_______________________________________________
Patches mailing list
Patches@python.org
http://mail.python.org/mailman/listinfo/patches
