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

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added a comment.

Thanks for having a look!



================
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;
 }
----------------
amccarth wrote:
> 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.
Good catch, I can try to fix that while fixing up the preexisting code here.


================
Comment at: libcxx/src/filesystem/filesystem_common.h:23
+#define NOMINMAX
+#include <windows.h>
+#else
----------------
amccarth wrote:
> 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?
> 
No, luckily this header is only used by the implementation itself, it doesn't end up polluting user code. This header is essentially only used by src/filesystem/operations.cpp and src/filesystem/directory_iterator.cpp for stuff shared by both of them.


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


================
Comment at: libcxx/src/filesystem/filesystem_common.h:182
+
+errc win_err_to_errc(DWORD err) {
+  for (const auto &pair : win_error_mapping)
----------------
amccarth wrote:
> Needs `inline` for ODR, right?
Hmm, I'm not entirely sure how this works, when this is an anonymous namespace (which essentially works as `static` in C afaik?), as that isn't supposed to be visible outside of the current translation unit (any similarity to similar named things in other translation units can be coincidental), so I'm not sure if such things can be made ODR... But this is the mechanism used for the header as it is right now.

If we can't get these shared between the two translation units in an anonymous namespace, I guess it'd be better to give at least the table a non-anonymous symbol so that it can be shared properly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91140