bpo-29548: Recommend PyObject_Call APIs over PyEval_Call APIs. #75

methane · 2017-02-13T17:38:08Z

PyEval_Call* APIs are not documented and they doesn't respect PY_SSIZE_T_CLEAN.
So add comment block to recommend them where PyEval_Call APIs are declared.

This commit also changes PyEval_CallMethod and PyEval_CallFunction implementation
same to PyObject_CallMethod and PyObject_CallFunction.

PyEval_CallFunction(callable, "i", (int)i) now calls callable(i) instead of raising TypeError.

I expect this allows compiler to share some code between PyEval_CallFunction and PyObject_CallFunction. But I'm not sure.

[bpo-29548]

Carreau · 2017-02-14T05:02:29Z

Include/ceval.h


-/* Inline this */
+/* Deprecated from Python 3.7: Use PyObject_CallObject() instead. */


did you ment Use PyObject_Call() it seem like this patch remove PyObject_CallObject ... Autocompleter got a bit overzealous ?

This patch deprecates PyEval_CallObject, not PyObject_CallObject.

Oh, sorry, I got confused with the unified diff and above comment, the proximity of PyObject_Call() and PyObject_CallObject() made me think you had an Object too much in the comment.

There are so many similar call APIs.
That's why I think we should deprecate undocumented and less used APIs.

codecov · 2017-02-14T14:41:46Z

Codecov Report

❗ No coverage uploaded for pull request base (master@c33ee85). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #75   +/-   ##
=========================================
  Coverage          ?   82.38%           
=========================================
  Files             ?     1428           
  Lines             ?   351138           
  Branches          ?        0           
=========================================
  Hits              ?   289281           
  Misses            ?    61857           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c33ee85...76737a6. Read the comment docs.

vstinner · 2017-02-15T23:49:00Z

Sorry but I'm confused by the PR title and the commit messages. They don't describe exactly what I see in the code. Can you please update them?

methane · 2017-02-16T00:16:23Z

I updated pull request title and body.

ncoghlan

For the PyEval_*/PyObject_* functions that are now duplicates, it would be useful to have block comments explaining the equivalence, and the fact the redundant definitions are being kept around for backwards compatibility reasons.

fixed

methane · 2017-02-28T06:34:52Z

@Haypo Would you review this again?

vstinner · 2017-02-28T20:14:53Z

Objects/call.c

    return PyObject_Call(callable, args, kwargs);
 }


 PyObject *
 PyObject_CallObject(PyObject *callable, PyObject *args)
 {
-    return PyEval_CallObjectWithKeywords(callable, args, NULL);


I don't understand why you change the code in this commit, and 2 commits later reverts this change. You should practice git rebase -i HEAD~4 to merge the two commits maybe?

vstinner · 2017-02-28T20:17:39Z

Objects/call.c

-    }
-
-    if (!PyTuple_Check(args)) {
+    if (args != NULL && !PyTuple_Check(args)) {


Please update the commit message which doesn't make sense compared to the modified code.

This commit fixes a bug, no? It makes sure that a TypeError is raised if kwargs is not a dictionary, right? Please explain it in the commit message.

I remove this bugfix for now and update #87 instead.

done. I created split issue for #87.

vstinner · 2017-02-28T20:18:08Z

Objects/call.c

    if (kwargs != NULL && !PyDict_Check(kwargs)) {
        PyErr_SetString(PyExc_TypeError,
                        "keyword list must be a dictionary");
        return NULL;
    }

+    if (args == NULL) {
+        return _PyObject_FastCallDict(callable, NULL, 0, kwargs);
+    }


maybe use an else here?

vstinner · 2017-02-28T20:18:45Z

Objects/call.c

@@ -942,22 +948,14 @@ PyObject_CallFunction(PyObject *callable, const char *format, ...)
 PyObject *
 PyEval_CallFunction(PyObject *callable, const char *format, ...)


commit message: this change also optimizes PyEval_CallFunction() and PyEval_CallMethod() to avoid temporary tuple when possible.

Main purpose is reducing maintenance cost rather than optimization.
There are very few callers in the world.

I hope compiler can remove duplicated code too, but I'm not sure.

vstinner · 2017-02-28T20:20:11Z

Include/ceval.h

@@ -7,6 +7,10 @@ extern "C" {

 /* Interface to random parts in ceval.c */

+/* PyObject_Call(), PyObject_CallFunction(), and PyObject_CallMethod()
+ * are documented and recommended API to call callable objects.


I don't understand the "are documented" part, maybe remove it?

I suggest to be more explicit:

" PyEval_CallObjectWithKeywords() is kept for backward compatibility: PyObject_Call(), PyObject_CallFunction() and PyObject_CallMethod() are recommended to call a callable object."

vstinner · 2017-02-28T20:24:26Z

I'm not confortable yet with GitHub to review commit messages :-/ It's not easy to comment the commit message: I also suggest to add "bpo-29548" to commit messages (at least to one commit message).

methane · 2017-03-01T08:17:44Z

I'm not confortable yet with GitHub to review commit messages :-/ It's not easy to comment the commit message:

I agree. I like Gerrit for this.

I also suggest to add "bpo-29548" to commit messages (at least to one commit message).

Final commit message is written when pushing "Squash and merge" button.
So I updated pull request title and body. The title will be first line of commit message, and the body follows.

I hope it works as poor man's Gerrit for a while, until we have new tool to build Misc/NEWS.

PyEval_Call* APIs are not documented and they doesn't respect PY_SSIZE_T_CLEAN. So add comment block to recommend PyObject_Call* APIs where they are declared. This commit also changes PyEval_CallMethod and PyEval_CallFunction implementation same to PyObject_CallMethod and PyObject_CallFunction to reduce future maintenance cost. Optimization to avoid temporary tuple are copied too. PyEval_CallFunction(callable, "i", (int)i) now calls callable(i) instead of raising TypeError. But accepting this edge case is backward compatible.

methane · 2017-03-01T09:33:04Z

I squashed all commit and modified commit message.

Co-authored-by: Jules <julia.poo.poo.poo@gmail.com>

Stay at eol when moving up/down

methane requested a review from vstinner February 13, 2017 17:38

the-knights-who-say-ni added the CLA signed label Feb 13, 2017

Carreau reviewed Feb 14, 2017

View reviewed changes

methane force-pushed the deprecate-evalcall branch from df67bdc to 89c7440 Compare February 14, 2017 14:32

methane changed the title ~~bpo-29548: deprecate PyEval_Call APIs.~~ bpo-29548: Stop using PyEval_Call* APIs. Feb 14, 2017

methane force-pushed the deprecate-evalcall branch from 89c7440 to 76737a6 Compare February 14, 2017 14:41

methane force-pushed the deprecate-evalcall branch 2 times, most recently from 04111c1 to 7b1dd6e Compare February 14, 2017 15:13

methane changed the title ~~bpo-29548: Stop using PyEval_Call* APIs.~~ bpo-29548: Recomend PyObject_Call APIs over PyEval_Call* APIs. Feb 16, 2017

ncoghlan changed the title ~~bpo-29548: Recomend PyObject_Call APIs over PyEval_Call* APIs.~~ bpo-29548: Recommend PyObject_Call APIs over PyEval_Call* APIs. Feb 19, 2017

ncoghlan previously requested changes Feb 19, 2017

View reviewed changes

methane mentioned this pull request Feb 20, 2017

Review dismissal python/core-workflow#36

Closed

vstinner requested changes Feb 28, 2017

View reviewed changes

methane changed the title ~~bpo-29548: Recommend PyObject_Call APIs over PyEval_Call* APIs.~~ bpo-29548: Recommend PyObject_Call APIs over PyEval_Call APIs. Mar 1, 2017

methane force-pushed the deprecate-evalcall branch 2 times, most recently from 3272dba to e17b495 Compare March 1, 2017 09:26

methane force-pushed the deprecate-evalcall branch from e17b495 to 4a3c0d5 Compare March 1, 2017 09:32

methane merged commit aa289a5 into python:master Mar 14, 2017

methane deleted the deprecate-evalcall branch March 14, 2017 09:01

methane mentioned this pull request Aug 30, 2022

Recommend PyObject_Call* APIs over PyEval_Call*() APIs #73734

Closed

jaraco pushed a commit that referenced this pull request Dec 2, 2022

Update aiohttp from 3.0.8 to 3.0.9 (#75)

81eb6cf

oraluben pushed a commit to oraluben/cpython that referenced this pull request Jul 15, 2023

docs: update readme with JIT (python#75)

5a26cb9

Co-authored-by: Jules <julia.poo.poo.poo@gmail.com>

gvanrossum mentioned this pull request Aug 22, 2023

heap-use-after-free in _PyFunction_LookupByVersion #108253

Closed

lysnikolaou pushed a commit to lysnikolaou/cpython that referenced this pull request May 1, 2024

Merge pull request python#75 from lysnikolaou/pyrepl-eol-move-up-down

390d778

Stay at eol when moving up/down

ngoldbaum mentioned this pull request Sep 23, 2024

Crash running PyO3 tests with --test-threads=1000 #124375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-29548: Recommend PyObject_Call APIs over PyEval_Call APIs. #75

bpo-29548: Recommend PyObject_Call APIs over PyEval_Call APIs. #75

methane commented Feb 13, 2017 •

edited

Loading

Carreau Feb 14, 2017

methane Feb 14, 2017

Carreau Feb 14, 2017

methane Feb 14, 2017

codecov bot commented Feb 14, 2017

vstinner commented Feb 15, 2017

methane commented Feb 16, 2017

ncoghlan left a comment •

edited by methane

Loading

methane commented Feb 28, 2017

vstinner Feb 28, 2017

vstinner Feb 28, 2017

methane Mar 1, 2017

methane Mar 1, 2017

vstinner Feb 28, 2017

vstinner Feb 28, 2017

methane Mar 1, 2017

vstinner Feb 28, 2017

methane Mar 1, 2017

vstinner commented Feb 28, 2017

methane commented Mar 1, 2017

methane commented Mar 1, 2017


		/* Inline this */
		/* Deprecated from Python 3.7: Use PyObject_CallObject() instead. */

		@@ -942,22 +948,14 @@ PyObject_CallFunction(PyObject callable, const char format, ...)
		PyObject *
		PyEval_CallFunction(PyObject callable, const char format, ...)

bpo-29548: Recommend PyObject_Call APIs over PyEval_Call APIs. #75

bpo-29548: Recommend PyObject_Call APIs over PyEval_Call APIs. #75

Conversation

methane commented Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 14, 2017

Codecov Report

vstinner commented Feb 15, 2017

methane commented Feb 16, 2017

ncoghlan left a comment • edited by methane Loading

Choose a reason for hiding this comment

methane commented Feb 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Feb 28, 2017

methane commented Mar 1, 2017

methane commented Mar 1, 2017

methane commented Feb 13, 2017 •

edited

Loading

ncoghlan left a comment •

edited by methane

Loading