Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.1k
bpo-46315: Use fopencookie() to avoid dup() in _PyTokenizer_FindEncodingFilename#32033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
bedevere-bot commentedMar 21, 2022
🤖 New build scheduled with the buildbot fleet by@pablogsal for commit6be12fc 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
Parser/tokenizer.c Outdated
| // cookie is just the fd | ||
| borrowed b; | ||
| b.fd = fd; | ||
| return fopencookie(b.cookie, "r", cookie_io); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fopencookie is a non-standard GNU extension. Is this available in all platforms targetted by web assembly? Does this need a configure check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I had a configure check in my first version of the patch. I got rid of it because it makes no sense. Both WASI and Emscripten use musl libc as upper half of libc and it always provides fdopencookie.
I wish their was a more elegant way to solve the problem._PyTokenizer_FindEncodingFilename uses a FILE pointer instead of a file descriptor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ugh, I am honestly not very happy with the amount of extra code, but I understand why is needed.
The failing tests on macOS ARM64 and FreeBSD are unrelated to my changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The code looks good to me, but I had no way to test it myself, so I am not formally approving. Feel free to land if you want though
Misc/NEWS.d/next/Core and Builtins/2022-03-21-20-05-10.bpo-46315.2QqFIC.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
I can shorten the code. |
Uh oh!
There was an error while loading.Please reload this page.
https://bugs.python.org/issue46315