Alexander Belopolsky <belopol...@users.sourceforge.net> added the comment:

Just a few nitpicks on the patch (in increasing pickiness):

1. Any reason to prefer PyTuple_SetItem to PyTuple_SET_ITEM at the end of 
_PyLong_Divmod_Near?   You are filling a brand new tuple, so PyTuple_SET_ITEM 
seems to be more appropriate.

2. temp = (quo_is_neg ? long_add : long_sub)(..) is clever, but IMO is less 
readable than 

if (quo_is_neg)
   temp = long_add(..)
else
   temp = long_sub(..)

The later form may also be more optimization friendly, particularly if compiler 
wants to inline static long_add or long_sub.

3. Given that arguments are named 'a' and 'b' it is a bit confusing to have 
local variable c of a different type.  I think 'cmp' would be a better choice.

4. I see that you removed a comment that displays round() implemented in 
python.  I think it would be helpful to preserve it for documentation and 
testing purposes even though the actual algorithm is slightly different.  As 
long as the results are the same, it is helpful to have reference python code.

5. Similarly to #4, having python implementation of divmod_near() displayed 
somewhere will be helpful.

----------
nosy: +belopolsky

_______________________________________
Python tracker <rep...@bugs.python.org>
<http://bugs.python.org/issue8817>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

Reply via email to