> 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.
