[libcxx-commits] [PATCH] D91140: [8/N] [libcxx] Fix the preexisting directory_iterator code for windows

[Adrian McCarthy via Phabricator via libcxx-commits]

amccarth requested changes to this revision.
amccarth added a comment.
This revision now requires changes to proceed.

This is pretty close.



================
Comment at: libcxx/src/filesystem/directory_iterator.cpp:86
+static uintmax_t get_file_size(const WIN32_FIND_DATAW& data) {
   return (data.nFileSizeHigh * (MAXDWORD + 1)) + data.nFileSizeLow;
 }
----------------
I know you didn't change this line, but it looks wrong to me.

`MAXDWORD` is a macro for `0xffffffff`, which I believe the compiler will interpret as `unsigned int` (or maybe `unsigned long`, which is only 32 bits in 64-bit Windows).  Adding 1 will cause it to wrap to 0.  Multiplying by 0 effectively ignores the higher bits, so we end up with just the low 32 bits of the size, which will be extended to a `uintmax_t` upon return.


================
Comment at: libcxx/src/filesystem/filesystem_common.h:23
+#define NOMINMAX
+#include <windows.h>
+#else
----------------
It's a real shame we have to include `<windows.h>` in a header.  Even with WIN32_LEAN_AND_MEAN and NOMINMAX, it really pollutes the macro and global namespaces.

Does code that includes `<filesystem>` transitively get this pulled into their code?  If so, would it be possible to move the definitions to a .cpp file and only include `<windows>` there?



================
Comment at: libcxx/src/filesystem/filesystem_common.h:128
+#if defined(_LIBCPP_WIN32API)
+const struct {
+  DWORD win;
----------------
`constexpr`?


================
Comment at: libcxx/src/filesystem/filesystem_common.h:182
+
+errc win_err_to_errc(DWORD err) {
+  for (const auto &pair : win_error_mapping)
----------------
Needs `inline` for ODR, right?


================
Comment at: libcxx/src/filesystem/filesystem_common.h:189
+
+error_code capture_last_error() {
+  return make_error_code(win_err_to_errc(GetLastError()));
----------------
`inline`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91140