[singlejar] Use Win32 API directly in OutputJar::AppendFile #6021
Conversation
Also remove unused pread implementation
|
@laszlocsomor With this PR, singlejar can run on Windows. |
src/tools/singlejar/output_jar.cc
Outdated
| while (static_cast<size_t>(total_written) < count) { | ||
| ssize_t len = std::min(kBufferSize, count - total_written); | ||
| DWORD n_read; | ||
| if (!::ReadFile(hFile, buffer.get(), len, &n_read, NULL)) |
There was a problem hiding this comment.
style: Please always use { and }, even for single statements. Omitting them is error-prone -- imagine the single statement in the if's body were a multi-statement macro.
Also in next two if-bodies.
| @@ -911,6 +918,20 @@ ssize_t OutputJar::AppendFile(int in_fd, off64_t offset, size_t count) { | |||
| } | |||
| ssize_t total_written = 0; | |||
There was a problem hiding this comment.
Why is total_written signed? Can you make it unsigned (size_t)?
There was a problem hiding this comment.
We will have to ask the original author. This function return total_written at the end and even the return type of this function is ssize_t.
There was a problem hiding this comment.
Thanks.
(I guess the reason is error reporting when count = 0. IMO, ReadFile()'s approach would be better -- i.e. return a boolean, report written amount to an argument.)
| @@ -911,6 +918,20 @@ ssize_t OutputJar::AppendFile(int in_fd, off64_t offset, size_t count) { | |||
| } | |||
| ssize_t total_written = 0; | |||
There was a problem hiding this comment.
Thanks.
(I guess the reason is error reporting when count = 0. IMO, ReadFile()'s approach would be better -- i.e. return a boolean, report written amount to an argument.)
preadis only used byOutputJar::AppendFile. ImplementingOutputJar::AppendFiledirectly with Win32 API means better performance and no worry about whetherpreadpolyfill follows POSIX spec correctly.Also remove unused
preadpolyfill.See #2241