[libcxx-commits] [PATCH] D151493: [libcxx] Handle windows system error code mapping in std::error_code.

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added a comment.

Looks mostly reasonable to me, but one rather significant issue about mapping between CRT and system error codes in the filesystem code.

One typo in the commit message, "winderr.h".

I don't quite follow all cases where errors can be compared directly and where they need to be canonicalized in the tests though.



================
Comment at: libcxx/src/filesystem/operations.cpp:424
     if ((fd = detail::open(p->c_str(), args...)) == -1) {
-      ec = capture_errno();
+      ec = detail::get_last_error();
       return FileDescriptor{p};
----------------
The `detail::open()` function uses `_wopen()`, which is a CRT function, which sets `errno`, not the windows error. (I guess it plausibly might leave the windows error code set too, but I guess we can't rely on it?)

So for all these cases where we switch from `capture_errno()` to `get_last_error()`, we'd need to make sure that we really do set the windows error code (or try to map from errno to windows error code).

(I guess the alternative, to keep track of which calls return in `errno` and which return in the windows error code and call the right function here, is unmaintainble.)


================
Comment at: libcxx/src/system_error.cpp:31
+#if defined(_LIBCPP_WIN32API)
+#  include <Windows.h>
+#  include <winerror.h>
----------------
Use lowercase `windows.h` for compatibility with mingw headers (which use lowercased header names consistently); while it is named `Windows.h` in the winsdk, the winsdk itself isn't self consistent and can't be used on a case sensitive filesystem without fixups in one form or another.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151493