Re: cpython: Fix email post-commit review comments.

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: cpython: Fix email post-commit review comments.

Antoine Pitrou
On Wed, 18 Apr 2012 15:31:10 +0200
brian.curtin <[hidden email]> wrote:

> http://hg.python.org/cpython/rev/bf23a6c215f6
> changeset:   76388:bf23a6c215f6
> parent:      76385:6762b943ee59
> user:        Brian Curtin <[hidden email]>
> date:        Wed Apr 18 08:30:51 2012 -0500
> summary:
>   Fix email post-commit review comments.
>
> Add INCREFs, fix args->kwargs, and a second args==NULL check was removed,
> left over from a merger with another function. Instead, checking msg==NULL
> does what that used to do in a roundabout way.

I don't think INCREFs were necessary, actually.
PyDict_SetItemString doesn't steal a reference.

(and here we see why reference-stealing APIs are a nuisance: because
you never know in advance whether a function will steal a reference or
not, and you have to read the docs for each and every C API call you
make)

Regards

Antoine.


_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%2B1324100855712-1801473%40n6.nabble.com
Reply | Threaded
Open this post in threaded view
|

Re: cpython: Fix email post-commit review comments.

Eric Snow-2
On Wed, Apr 18, 2012 at 8:21 AM, Antoine Pitrou <[hidden email]> wrote:
> (and here we see why reference-stealing APIs are a nuisance: because
> you never know in advance whether a function will steal a reference or
> not, and you have to read the docs for each and every C API call you
> make)

+1

-eric
_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%2B1324100855712-1801473%40n6.nabble.com
Reply | Threaded
Open this post in threaded view
|

Re: cpython: Fix email post-commit review comments.

Gregory Ewing
In reply to this post by Antoine Pitrou
Antoine Pitrou wrote:

> (and here we see why reference-stealing APIs are a nuisance: because
> you never know in advance whether a function will steal a reference or
> not, and you have to read the docs for each and every C API call you
> make)

Fortunately, they're very rare, so you don't encounter
them often.

Unfortunately, they're very rare, so you're all the more
likely to forget about them and get bitten.

Functions with ref-stealing APIs really ought to have
a naming convention that makes them stand out and remind
you to consult the documentation.

--
Greg
_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%2B1324100855712-1801473%40n6.nabble.com
Reply | Threaded
Open this post in threaded view
|

Re: cpython: Fix email post-commit review comments.

Tres Seaver
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/18/2012 06:48 PM, Greg Ewing wrote:

> Functions with ref-stealing APIs really ought to have a naming
> convention that makes them stand out and remind you to consult the
> documentation.

Maybe we should mandate that their names end with '_rtfm'.


Tres.
- --
===================================================================
Tres Seaver          +1 540-429-0999          [hidden email]
Palladion Software   "Excellence by Design"    http://palladion.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk+PTLwACgkQ+gerLs4ltQ5YgACg17rdlCVf8YJmGoYP2eANC8ya
RhoAnimJr/5FzR59IELHAyhdXOO1c+uJ
=uWHZ
-----END PGP SIGNATURE-----

_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%2B1324100855712-1801473%40n6.nabble.com
Reply | Threaded
Open this post in threaded view
|

Highlighting reference-stealing APIs [was Re: cpython: Fix email post-commit review comments.]

David Malcolm
In reply to this post by Gregory Ewing
On Thu, 2012-04-19 at 10:48 +1200, Greg Ewing wrote:

> Antoine Pitrou wrote:
>
> > (and here we see why reference-stealing APIs are a nuisance: because
> > you never know in advance whether a function will steal a reference or
> > not, and you have to read the docs for each and every C API call you
> > make)
>
> Fortunately, they're very rare, so you don't encounter
> them often.
>
> Unfortunately, they're very rare, so you're all the more
> likely to forget about them and get bitten.
>
> Functions with ref-stealing APIs really ought to have
> a naming convention that makes them stand out and remind
> you to consult the documentation.
FWIW my refcount static analyzer adds various new compile-time
attributes to gcc:
http://gcc-python-plugin.readthedocs.org/en/latest/cpychecker.html#marking-functions-that-steal-references-to-their-arguments
so you can write declarations like these:

extern void bar(int i, PyObject *obj, int j, PyObject *other)
  CPYCHECKER_STEALS_REFERENCE_TO_ARG(2)
  CPYCHECKER_STEALS_REFERENCE_TO_ARG(4);

There's a similar attribute for functions that return borrowed
references:

  PyObject *foo(void)
    CPYCHECKER_RETURNS_BORROWED_REF;

Perhaps we should add such attributes to the headers for Python 3.3?
(perhaps with a different naming convention?)

Hope this is helpful
Dave

_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%2B1324100855712-1801473%40n6.nabble.com
Reply | Threaded
Open this post in threaded view
|

Re: cpython: Fix email post-commit review comments.

Gregory Ewing
In reply to this post by Tres Seaver
On 19/04/12 11:22, Tres Seaver wrote:

> Maybe we should mandate that their names end with '_rtfm'.

+1

--
Greg
_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%2B1324100855712-1801473%40n6.nabble.com
Reply | Threaded
Open this post in threaded view
|

Re: Highlighting reference-stealing APIs [was Re: cpython: Fix email post-commit review comments.]

Gregory P. Smith-3
In reply to this post by David Malcolm


On Wed, Apr 18, 2012 at 5:01 PM, David Malcolm <[hidden email]> wrote:
On Thu, 2012-04-19 at 10:48 +1200, Greg Ewing wrote:
> Antoine Pitrou wrote:
>
> > (and here we see why reference-stealing APIs are a nuisance: because
> > you never know in advance whether a function will steal a reference or
> > not, and you have to read the docs for each and every C API call you
> > make)
>
> Fortunately, they're very rare, so you don't encounter
> them often.
>
> Unfortunately, they're very rare, so you're all the more
> likely to forget about them and get bitten.
>
> Functions with ref-stealing APIs really ought to have
> a naming convention that makes them stand out and remind
> you to consult the documentation.
FWIW my refcount static analyzer adds various new compile-time
attributes to gcc:
http://gcc-python-plugin.readthedocs.org/en/latest/cpychecker.html#marking-functions-that-steal-references-to-their-arguments
so you can write declarations like these:

extern void bar(int i, PyObject *obj, int j, PyObject *other)
 CPYCHECKER_STEALS_REFERENCE_TO_ARG(2)
 CPYCHECKER_STEALS_REFERENCE_TO_ARG(4);

There's a similar attribute for functions that return borrowed
references:

 PyObject *foo(void)
   CPYCHECKER_RETURNS_BORROWED_REF;

Perhaps we should add such attributes to the headers for Python 3.3?
(perhaps with a different naming convention?)

+1  Adding these annotations and setting up a buildbot that builds using cpychecker would be a great.

-gps
 

Hope this is helpful
Dave

_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/greg%40krypto.org


_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%2B1324100855712-1801473%40n6.nabble.com
Reply | Threaded
Open this post in threaded view
|

Re: cpython: Fix email post-commit review comments.

Nick Coghlan
In reply to this post by Antoine Pitrou
On Thu, Apr 19, 2012 at 12:21 AM, Antoine Pitrou <[hidden email]> wrote:
> I don't think INCREFs were necessary, actually.
> PyDict_SetItemString doesn't steal a reference.

Yes, I was tired when that checkin went by and my brain didn't
register that the function was otherwise using borrowed refs for name
and path, so it was also correct to use borrowed refs to Py_None.

I should have been less cryptic and written out my full question
"Should there be Py_INCREF's here?" rather than using the shorthand (i
genuinely wasn't sure at the time, but that wasn't clear from what I
wrote).

> (and here we see why reference-stealing APIs are a nuisance: because
> you never know in advance whether a function will steal a reference or
> not, and you have to read the docs for each and every C API call you
> make)

Yeah, it would have been nice if there was an explicit hint in the API
names when reference stealing was involved, but I guess it's far too
late now :(

Regards,
Nick.

--
Nick Coghlan   |   [hidden email]   |   Brisbane, Australia
_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%2B1324100855712-1801473%40n6.nabble.com
Reply | Threaded
Open this post in threaded view
|

Re: Highlighting reference-stealing APIs [was Re: cpython: Fix email post-commit review comments.]

Nick Coghlan
In reply to this post by Gregory P. Smith-3
On Thu, Apr 19, 2012 at 11:04 AM, Gregory P. Smith <[hidden email]> wrote:
> +1  Adding these annotations and setting up a buildbot that builds using
> cpychecker would be a great.

Even without the extra annotations, running cpychecker on at least one
of the buildbots might be helpful.

I'm in the process of setting up a buildbot for RHEL 6, once I get it
up and running normally, I'll look into what it would take to install
and enable cpychecker for the builds. (Or, alternatively, I may make
it a separate cron job, similar to the daily refcount leak detection
run).

Cheers,
Nick.

--
Nick Coghlan   |   [hidden email]   |   Brisbane, Australia
_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%2B1324100855712-1801473%40n6.nabble.com
Reply | Threaded
Open this post in threaded view
|

Re: Highlighting reference-stealing APIs [was Re: cpython: Fix email post-commit review comments.]

Sam Partington
On 19 April 2012 02:20, Nick Coghlan <[hidden email]> wrote:
> On Thu, Apr 19, 2012 at 12:21 AM, Antoine Pitrou <[hidden email]> wrote:
>> (and here we see why reference-stealing APIs are a nuisance: because
>> you never know in advance whether a function will steal a reference or
>> not, and you have to read the docs for each and every C API call you
>> make)
>
> Yeah, it would have been nice if there was an explicit hint in the API
> names when reference stealing was involved, but I guess it's far too
> late now :(

It's too late to change the fn names sure, but you could change the
argument names in question for reference stealing apis with some kind
of markup.

That would make it fairly easy to write a script that did the checking for you :

int PyTuple_SetItem(PyObject *p, Py_ssize_t pos, PyObject *stolen_o)

Or better yet would be to mark the types :

int PyTuple_SetItem(PyObject *p, Py_ssize_t pos, PyStolenObject* o)
PyBorrowedObject* PyTuple_GetItem(PyObject *p, Py_ssize_t pos)

PyStolenObject and PyBorrowedObject would just be typedefs to PyObject
normally. But a consenting user could define PyENABLE_CHECKED_REFS
before including Python.h which would given

#if defined(PyENABLE_CHECKED_STOLEN_REFS)
struct PyStolenObject;
struct PyBorrowedObject;
#define PyYesIKnowItsStolen(o) ((PyStolenObject*)o)
#define PyYesIKnowItsBorrowed(o) ((PyObject*)o)
#else
typedef PyStolenObject PyObject;
typedef PyBorrowedObject PyObject;
#endif

Forcing the user to use

PyTuple_SetItem(p, pos, PyYesIKnowItsStolen(o))
PyObject * ref = PyYesIKnowItsBorrowed(PyTuple_GetItem(p, pos));

Or else it would fail to compile.  The user could even add her own :

PyStolenObject* IncRefBecauseIKnowItsStolen(PyObject* o) {
PyIncRef(o); return (PyStolenObject*)o; }
PyObject* IncRefBecauseIKnowItsBorrowed(PyBorrowedObject* o) {
PyIncRef(o); return (PyObject*)o; }

This would not require a special gcc build and would be available to
anyone who wanted it. I use a similar, C++ based trick in my python
extension code to avoid the whole issue of ref leaking, but I have to
be careful at the point of calling the python api, having it automatic
would be great.

On a similar note, I have just implemented a wrapper around Python.h
which runtime checks that the GIL is held around every call to the
Python API or else fails very noisily. This was done because it turns
out that wxPython had a ton of non-GIL calls to the API causing random
sporadic set faults in our app.  We now use it on several of our
extensions.  It doesn't require any changes to Python.h,  you just
need to add an include path before the python include path. Would
there be any interest in this?

Sam


On 19 April 2012 02:25, Nick Coghlan <[hidden email]> wrote:

> On Thu, Apr 19, 2012 at 11:04 AM, Gregory P. Smith <[hidden email]> wrote:
>> +1  Adding these annotations and setting up a buildbot that builds using
>> cpychecker would be a great.
>
> Even without the extra annotations, running cpychecker on at least one
> of the buildbots might be helpful.
>
> I'm in the process of setting up a buildbot for RHEL 6, once I get it
> up and running normally, I'll look into what it would take to install
> and enable cpychecker for the builds. (Or, alternatively, I may make
> it a separate cron job, similar to the daily refcount leak detection
> run).
>
> Cheers,
> Nick.
>
> --
> Nick Coghlan   |   [hidden email]   |   Brisbane, Australia
> _______________________________________________
> Python-Dev mailing list
> [hidden email]
> http://mail.python.org/mailman/listinfo/python-dev
> Unsubscribe: http://mail.python.org/mailman/options/python-dev/sam.partington%40gmail.com
_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%2B1324100855712-1801473%40n6.nabble.com
Reply | Threaded
Open this post in threaded view
|

Re: Highlighting reference-stealing APIs [was Re: cpython: Fix email post-commit review comments.]

"Martin v. Löwis"
Am 19.04.2012 12:42, schrieb Sam Partington:

> On 19 April 2012 02:20, Nick Coghlan <[hidden email]> wrote:
>> On Thu, Apr 19, 2012 at 12:21 AM, Antoine Pitrou <[hidden email]> wrote:
>>> (and here we see why reference-stealing APIs are a nuisance: because
>>> you never know in advance whether a function will steal a reference or
>>> not, and you have to read the docs for each and every C API call you
>>> make)
>>
>> Yeah, it would have been nice if there was an explicit hint in the API
>> names when reference stealing was involved, but I guess it's far too
>> late now :(
>
> It's too late to change the fn names sure, but you could change the
> argument names in question for reference stealing apis with some kind
> of markup.

While it may too late to change the names, it's not to late to remove
these functions entirely. It will take some time, but it would be
possible to add parallel APIs that neither borrow nor steal references,
and have them preferred over the existing APIs. Then, with Python 4,
the old APIs could go away.

Regards,
Martin
_______________________________________________
Python-Dev mailing list
[hidden email]
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/lists%2B1324100855712-1801473%40n6.nabble.com