[PATCH] D66344: Filesystem/Windows: fix inconsistency in readNativeFileSlice API

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: lib/Support/Windows/Path.inc:1233
+    // EOF is not an error.
+    if (Err == ERROR_BROKEN_PIPE || Err == ERROR_HANDLE_EOF)
       return std::error_code();
----------------
aganea wrote:
> Zachary also handles `ERROR_OPERATION_ABORTED` in `lldb/source/Host/windows/ConnectionGenericFileWindows.cpp` which corresponds to the user hitting Ctrl-C.
> 
> Also small nit not related to your change: we can't discriminate between a "full success" and a "eof success" from outside of this API, which could cause an extra loop to execute below? (and possibly trigger a different return code in `readNativeFileSlice`)
Handling ctrl C seems reasonable to me, but it's not unit testable, so if you want to do it separately and manually test it, that's fine with me too.

I guess the concern with the loop for slice reading is that successive calls to ReadFile may not return the same error code. Right now, the loop is basically dead code since no test reads a slice of more than 4GB of data so far as I know, so it's hard to know what happens. Prior to my changes, I think the loop structure was exactly the same, though: it called `::read` until it got back EOF or zero bytes read. We could check what `read` does in the CRT to see what we should be doing.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66344/new/

https://reviews.llvm.org/D66344