Good work - sent new review http://codereview.appspot.com/6305077/
python is currently single threaded so you could use a static var here, though I would like to avoid this where possible. On Sat, Jun 9, 2012 at 12:06 PM, Antonio Ospite <[email protected]> wrote: > On Thu, 7 Jun 2012 16:18:03 +0200 > Campbell Barton <[email protected]> wrote: > >> code review, patch neesd some more work. >> http://www.pasteall.org/pic/show.php?id=32822 > > OK, the code looks a lot better now, thanks! > Patch attached, along with the test script I am using. > > These are the changes since the v1: > > - Move some variables in an internal scope > - Allow sorting by the existing index order > - Decrement 'index' and 'py_elem' to avoid memory leaks > - Don't check the return value of BPy_BMElem_CreatePyObject() > - Use PyNumber_Check to verify that keyfunc actually returns a number > - Use the helper function range_vn_i() to intialize elem_idx > - Check that the key function is actually a callable object > - Check the return value of PyObject_CallFunctionObjArgs > - Move allocations right before where the memory is used > - Use qsort_r to sort the indices by the values in the 'keys' array > - Use PyMem_MALLOC and PyMem_FREE instead of malloc and free > - Return NULL on error and Py_RETURN_NONE on success, cleanup in > either case > - Call PyErr_NoMemory when allocations fail > - Set python exceptions appropriately on error paths > - Remove ultra-verbose debug printouts > - Avoid sorting if the sequence has 0 or 1 elements > - Reorder declarations, and remove initializers when not strictly > needed > - Cleanup docs and comments > > There is still a problem with the current code, I think it will only > work on GNU systems: qsort_r does not seem to be that portable, on some > other Unix systems the compare function has a different order of > parameters[1] and on windows qsort_s[2] should be used, again with a > different order of parameters in the compare function. > > I can think to 3 solutions: > - just use 'qsort' with 'keys' as a global variable, I don't know what > are the assumptions in blender about multi-threading, and if that is > going to cause problems. > > - add some wrapping of qsort_r to handle system differences: boring and > error prone? > > - build up a PyList and use that for sorting: overhead? > > Thanks, > Antonio > > [1] http://sourceware.org/ml/libc-alpha/2008-12/msg00003.html > [2] http://msdn.microsoft.com/en-us/library/4xc60xas%28VS.80%29.aspx > > -- > Antonio Ospite > http://ao2.it > > A: Because it messes up the order in which people normally read text. > See http://en.wikipedia.org/wiki/Posting_style > Q: Why is top-posting such a bad thing? -- - Campbell _______________________________________________ Bf-python mailing list [email protected] http://lists.blender.org/mailman/listinfo/bf-python
