It's not clear to me that Py_UNICODE is guaranteed to be wchar_t for all time, that's all. If it is, go ahead. Otherwise the path_t object has the ability to clean up after itself, so perhaps it should be used here?
It's not clear to me that Py_UNICODE is guaranteed to be wchar_t for all time, that's all. If it is, go ahead.
Right now, it's the case and path_converter() leaks memory, so I
proposed a simple and obvious fix :-)
On Windows, it makes sense to continue to use the UTF-16 encoded
string cached in Unicode objects, because this conversion is common,
and it's convenient to not have to release the memory explicitly. The
side effect is that we may waste memory in some cases.
Otherwise the path_t object has the ability to clean up after itself, so perhaps it should be used here?
Maybe, but I'm not interested to write a more complex patch for
Windows :-) Try to just call PyMem_Free(path->wide) (do nothing it's
The advantage of the old code (and my patch) is to only convert a
filename once to UTF-16 and then use the cached result.
path_converter.patch LGTM for 3.6 (and 3.7, 3.8), but we should find better solution for future versions. Could you please add a comment that PyUnicode_AsUnicodeAndSize is deprecated, but used because it is the simplest and the most efficient way for now?
Feel free to modify the code if you see a better way to encode paths on Windows ;-) But it's a much larger project, and I'm not really interested to jump again in this silly deprecated APIs :-) It seems like the status quo is that Py_UNICODE is going to stay longer than expected and since it "just works", nobody really cares :-)
I close this issue since the initial bug (memory leak) is now fixed.
Misc/NEWSso that it is managed by towncrier #552
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
The text was updated successfully, but these errors were encountered: