Author zach.ware
Recipients larry, nadeem.vawda, serhiy.storchaka, vajrasky, zach.ware
Date 2014-02-03.22:26:35
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1391466395.35.0.827926988796.issue20185@psf.upfronthosting.co.za>
In-reply-to
Content
Vajrasky Kok wrote:
> However, there are some reviews that I could not implement.
>
> 1. "This is a good candidate for a custom return converter."
>
> I can not synchronize struct rlimit and NULL return values.

Looking again, that one is non-trivial, but still doable.  You just need a "this means an error happened" value to initialize rl to, and return that value instead of NULL.

> 2. "Should be 'class float "PyFloatObject *" "&PyFloat_Type"'.  Using
> PyFloatObject * instead of PyObject * may require some casts to
> PyObject * in some places, but it's better to use the real name."
>
> I tried it but it was like opening pandora box. It's too much effort
> to surpress compile errors and warnings. And casting PyFloatObject
> to PyObject in many places, such as functions, macros, makes me
> nervous. I think this one deserves a dedicated ticket.

It didn't look too bad to me.  There are already several places where a value is cast back and forth between PyObject and PyFloatObject.  Giving 'self' the right type allows a couple of casts to be removed, and the ones that have to be added are almost exclusively in calls to PyFloat_AsDouble or the CONVERT_TO_DOUBLE macro (which looks like it can just do the casts itself without much issue).  I would vote for making PyFloat_AsDouble expect PyFloatObject instead of PyObject, but since (I think?) it looks like it's part of the stable ABI, I'm not sure if that would fly.

See http://hg.python.org/sandbox/zware/rev/51473d8c23f8 for a patch on your patch that uses PyFloatObject, compiles cleanly (on win32, at least), and passes relevant tests (though I haven't run the full test suite on this yet; it takes forever on this PC).

I have a few more review comments that I hope to get posted later this evening.

The patch is looking pretty good overall, though!
History
Date User Action Args
2014-02-03 22:26:35zach.waresetrecipients: + zach.ware, larry, nadeem.vawda, serhiy.storchaka, vajrasky
2014-02-03 22:26:35zach.waresetmessageid: <1391466395.35.0.827926988796.issue20185@psf.upfronthosting.co.za>
2014-02-03 22:26:35zach.warelinkissue20185 messages
2014-02-03 22:26:35zach.warecreate