-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
change sopen_s to wsopen_s (fmtlib#3234) #3293
Conversation
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.
Thanks for the PR. Overall looks good, just a few minor comments inline.
test/posix-mock.h
Outdated
errno_t wsopen_s(int* pfh, const wchar_t* filename, int oflag, int shflag, | ||
int pmode); |
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 don't think this is needed because there is no corresponding test in posix-mock-test. You'll need to remove FMT_POSIX_CALL
around the call to wsopen_s
as well.
src/os.cc
Outdated
using mode_t = int; | ||
constexpr mode_t mode = | ||
S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH; | ||
FMT_POSIX_CALL(wsopen_s(&fd_, path.c_str(), oflag, _SH_DENYNO, mode)); |
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.
Looks like wsopen_s returns an error code so I suggest using it instead of global errno.
src/os.cc
Outdated
int fd_ = -1; | ||
using mode_t = int; | ||
constexpr mode_t mode = | ||
S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH; |
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.
Let's move to a constant, e.g. default_open_mode
and reuse here and above to make sure modes are consistent.
It seems windows doesn't return EINTR on _wsopen_s, so I removed |
src/os.cc
Outdated
fd_ = -1; | ||
FMT_POSIX_CALL(sopen_s(&fd_, path.c_str(), oflag, _SH_DENYNO, mode)); | ||
auto converted = detail::utf8_to_utf16(string_view(path.c_str())); | ||
auto err = | ||
_wsopen_s(&fd_, converted.c_str(), oflag, _SH_DENYNO, default_open_mode); |
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 think we could avoid a bit of duplication by calling open_windows_file
here and moving its result into *this
.
Thank you |
Fix #3234.