On Mon, May 27, 2013 at 9:54 PM, ZyX wrote: >> The patch is not correct: >> >> 'bytes' is declared in OptionsAssItem(), in the block (line 1689): >> >> 'else if (PyUnicode_Check(valObject))' >> >> This declaration shadows the declaration of 'bytes' made by DICTKEY_DECL at >> the start of the function. Hence the shadowed 'bytes' PyObject is never >> decreferenced whenever OptionsAssItem() returns from this block. > > No longer the case after switching to StringToChars and using > dictkey_todecref as a name. > >> The DICTKEY_ family of macros have been introduced by patch v7-3-569. It >> seems >> that all these macros should be removed. >> >> DICTKEY_DECL: >> Clearly a dangerous macro when the author of the macro himself can be fooled >> by >> the fact that the macro hides a declaration. > > It is not the only one e.g. Py_BEGIN_ALLOW_THREADS does the same. Hiding > common `bytes` declaration is no good, but specific `dictkey_todecref` is OK. > >> DICTKEY_UNREF: >> The macro should not test for 'bytes != NULL' as Py_XDECREF's purpose is >> specifically to do this test before doing the decreferencing. > > Was fixed in some later patches. > >> This macro hides a decreferencing and hides the object that is being >> decreferenced. See >> http://docs.python.org/2/extending/extending.html#reference-counting-in-python >> for how critical it is to get the reference counting correct when >> using the python/C API, to avoid crashes and memory leaks. > > It does not matter much what to write, DICTKEY_UNREF or Py_(X)DECREF. What > matters is that in some places you cannot just return err; you need to decref > other objects as well. > >> The 'bytes' object must be decreferenced as soon as not needed anymore, which >> means inside the DICTKEY_GET macro and after the call to >> PyString_AsStringAndSize(). So the macro is actually never needed. > > Nope, check out documentation. If it was so simple I would have used a > function: due to > >> The buffer refers to an internal string buffer of obj, not a copy. > > I *must* keep a reference until I no longer need key. Or save a copy instead > which is bad for performance. (I am using function accepting return-argument > PyObject ** now though: it is set to an objects that is to be Py_XDECREFed.) > >> DICTKEY_GET: >> This is a macro with parameters, but missing most parameters (keyObject, key, >> bytes) and therefore very obscure. >> >> Its name is misleading, the purpose of the macro is to set a pointer to an >> internal null terminated string that represents a string, a Unicode or a >> bytes >> python object and there is no reference to a dictionary or of one of its >> keys. >> >> Getting such a pointer is obviously often needed to interface vim with python >> and this means that it should be a function. And indeed there are many other >> parts in the code where this is implemented (however not consistently) and >> that >> would benefit from using this function, for example: >> >> . no need to look very far, right in the 'else if >> >> (PyUnicode_Check(valObject))' block mentioned above, but here the >> implementation forgets to use CODEC_ERROR_HANDLER (this is a bug) > > Please refer to the latest patch in sequence. There *is* now a function. > >> . at the start of StringToLine() but here there is no error handling (this >> is >> a bug) and the weird handling of 'bytes' in the case of Python 2 could be >> avoided by using this function > > This one is true, I usually avoid touching built-in functions. > > -- > -- > You received this message from the "vim_dev" maillist. > Do not top-post! Type your reply below the text you are replying to. > For more information, visit http://www.vim.org/maillist.php > > --- > You received this message because you are subscribed to the Google Groups > "vim_dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > For more options, visit https://groups.google.com/groups/opt_out. > >
You seem to imply in your answer to my patch review, that in order to review one of your patch, one should read all the (not yet accepted) patches following this patch. So there are patches that fix patches that fix patches in the python interface :-( This constraint makes any patch review useless, since you can never be sure that the review you are doing is not overriden by one of your batch of patches submitted the next day. Also I note that in your answer, in most cases, you do not even bother to give the exact reference to the patch that is supposed to fix the problem raised in this patch. I was just trying to get a feeling about the soundness of the python interface before writing anything about multi-threading, and I thought that reviewing all the bug-fixing patches in the last serie would be a good way to do that. Actually the first patch was enough. -- Xavier Les Chemins de Lokoti: http://lokoti.alwaysdata.net -- -- You received this message from the "vim_dev" maillist. Do not top-post! Type your reply below the text you are replying to. For more information, visit http://www.vim.org/maillist.php --- You received this message because you are subscribed to the Google Groups "vim_dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/groups/opt_out.
