sort with a key speed patch - Code Review (original) (raw)

Looks good generally, but many small things (see below). Also, I find the splitting into incremental patches quite detrimental since the various patches are hardly independent.

http://codereview.appspot.com/3269041/diff/1/Objects/listobject.c File Objects/listobject.c (right):

http://codereview.appspot.com/3269041/diff/1/Objects/listobject.c#newcode1446 Objects/listobject.c:1446: merge_freemem(ms); /* reset to sane state */ Is there any reason to remove this?

http://codereview.appspot.com/3269041/diff/2001/Objects/listobject.c#newcode1027 Objects/listobject.c:1027: static int I don't think there's any point in explicit inlining of such non-trivial routines. Just let the compiler do its job, IMO.

http://codereview.appspot.com/3269041/diff/2001/Objects/listobject.c#newcode1099 Objects/listobject.c:1099: Returns -1 in case of error. Same comment about explicit inlining.

http://codereview.appspot.com/3269041/diff/2001/Objects/listobject.c#newcode1383 Objects/listobject.c:1383: ms->a.keys = ms->temparray; A comment would be in order here. Are you trying to keep everything contiguous to maximize spatial locality?

http://codereview.appspot.com/3269041/diff/2001/Objects/listobject.c#newcode1496 Objects/listobject.c:1496: sortslice_copy_incr(&dest, &ssb); Please don't do such gratuitous style changes (especially given that the new style is worse than the old one).

http://codereview.appspot.com/3269041/diff/2001/Objects/listobject.c#newcode1787 Objects/listobject.c:1787: * until the stack invariants are re-established: Same comment about explicit inlining (!).

http://codereview.appspot.com/3269041/diff/2001/Objects/listobject.c#newcode1816 Objects/listobject.c:1816: } Same comment about explicit inlining (!) :-)))

http://codereview.appspot.com/3269041/diff/2001/Objects/listobject.c#newcode1858 Objects/listobject.c:1858: r |= n & 1; Any point in this change? Since you are requesting inlining I don't understand what it brings.

http://codereview.appspot.com/3269041/diff/2001/Objects/listobject.c#newcode1922 Objects/listobject.c:1922: This is starting to look cryptic (or at least non-obvious). Can you add a comment? (or switch back to the simpler stack_keys approach)

http://codereview.appspot.com/3269041/diff/4001/Objects/listobject.c#newcode1049 Objects/listobject.c:1049: * The second is vacuously true at the start. This change looks gratuitous. Left-hand and right-hand code seem to do the exact same thing.

http://codereview.appspot.com/3269041/diff/4001/Objects/listobject.c#newcode1494 Objects/listobject.c:1494: if (k < 0) Looks quite gratuitous too. Did you measure any significant improvement?

(ditto for the couple next changes in this patch)

http://codereview.appspot.com/3269041/diff/18001/Objects/listobject.c#newcode... Objects/listobject.c:1036: /* assert [lo, start) is sorted */ Seems to me that "hi" was more descriptive here.

http://codereview.appspot.com/3269041/diff/20001/Objects/listobject.c#newcode... Objects/listobject.c:1447: return -1; What is "pa" here?

http://codereview.appspot.com/3269041/diff/20001/Objects/listobject.c#newcode... Objects/listobject.c:1572: Fail: Again, what is "pa"?