Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(15690)

#7830: Flatten nested functools.partial

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 11 months ago by belopols
Modified:
4 years, 11 months ago
Reviewers:
shadowranger+python
CC:
rhettinger, sasha, AntoinePitrou, haypo, jackdied, hsoft_hardcoded.net, Christophe Simonis, devnull_psf.upfronthosting.co.za, Josh.R
Visibility:
Public.

Patch Set 1 #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/functools.py View 1 chunk +8 lines, -0 lines 0 comments Download
Lib/test/test_functools.py View 1 chunk +10 lines, -0 lines 0 comments Download
Modules/_functoolsmodule.c View 3 chunks +48 lines, -7 lines 6 comments Download

Messages

Total messages: 1
Josh.R
4 years, 11 months ago #1
Mostly nitpicks, but there is one important issue (a fresh tuple is leaked in
the case where PySequence_Concat fails).

http://bugs.python.org/review/7830/diff/13009/Modules/_functoolsmodule.c
File Modules/_functoolsmodule.c (right):

http://bugs.python.org/review/7830/diff/13009/Modules/_functoolsmodule.c#newc...
Modules/_functoolsmodule.c:26: partial_new(PyTypeObject *type, PyObject *args,
PyObject *kw)
Might it make sense to normalize kw to NULL if it's passed as an empty dict?
Reduces the possible states to handle when making wrapping optimizations work
correctly. I know it's weird, but I was just noticing that:
i = partial(int, **{})
will actually pass an empty dict and initialize the keywords property as a dict
instead of None.

http://bugs.python.org/review/7830/diff/13009/Modules/_functoolsmodule.c#newc...
Modules/_functoolsmodule.c:41: if (part->dict == NULL) {
Should you also allow this seamless behavior to work when dict is the empty
dictionary? If any random inspection utility happens to read the __dict__
attribute, it will auto-vivify it, preventing this optimization from working.

http://bugs.python.org/review/7830/diff/13009/Modules/_functoolsmodule.c#newc...
Modules/_functoolsmodule.c:81: return NULL;
I think you need to DECREF nargs here. It's not assigned to pto, so DECREF-ing
pto isn't clearing it.

http://bugs.python.org/review/7830/diff/13009/Modules/_functoolsmodule.c#newc...
Modules/_functoolsmodule.c:84: Py_DECREF(nargs);
Replacement for lines 68 to 84  to fix ref counting and avoid excess DECREF work
when we're keeping nargs might be:
if (pargs == Py_None || PyTuple_GET_SIZE(pargs) == 0) {
    pto->args = nargs;
}
else if (PyTuple_GET_SIZE(nargs) == 0) {
    pto->args = pargs;
    Py_INCREF(pargs);
    Py_DECREF(nargs);
}
else {
    pto->args = PySequence_Concat(pargs, nargs);
    Py_DECREF(nargs);
    if (pto->args == NULL) {
        pto->kw = NULL;
        Py_DECREF(pto);
	return NULL;
    }
}

http://bugs.python.org/review/7830/diff/13009/Modules/_functoolsmodule.c#newc...
Modules/_functoolsmodule.c:88: pto->kw = PyDict_Copy(kw);
Is this copy needed? Maybe I'm mistaken, but I thought the keyword argument dict
received by a function was always freshly created (it behaves that way using
call(**kwds) at the Python level, but I won't swear to this behavior at the C
level; PyObject_Call may just pass the provided dictionary directly). Note that
normalizing empty dictionaries to NULL as suggested on line 26 will ensure the
(hypothetical) cheap INCREF path is taken here because partial objects without
keyword arguments would always be Py_None after construction, never empty dict.

http://bugs.python.org/review/7830/diff/13009/Modules/_functoolsmodule.c#newc...
Modules/_functoolsmodule.c:106: Py_INCREF(pkw);
You might want to copy pkw in this case, not just copy the reference. The
keywords dictionary is mutable (which is its own bit of weirdness) on partial
objects; if you just store a new reference to it, you could change the "wrapper"
(now rewrapped) partial object by modifying the "inner" (now "original") partial
object. That's roughly how the current implementation behaves, but unlike the
current implementation, the mutations would work both ways; if you didn't add
new keywords when you wrapped, then modifying the new partial object would
change the old one too (assuming references were kept to both).

Either way it's a minor breaking change, but I'm really hoping no one was
relying on evil stuff like that... Copying the dictionary is a little more
costly, but it reduces the risk of any spooky side-effects if someone makes a
mistake.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+