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

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo marked 2 inline comments as done.
mstorsjo added inline comments.


================
Comment at: libcxx/src/filesystem/filesystem_common.h:23
+#define NOMINMAX
+#include <windows.h>
+#else
----------------
mstorsjo wrote:
> 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.
I removed the need for windows.h in the shared header anyway, without excessive jumping through hoops.


================
Comment at: libcxx/src/filesystem/filesystem_common.h:182
+
+errc win_err_to_errc(DWORD err) {
+  for (const auto &pair : win_error_mapping)
----------------
mstorsjo wrote:
> 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.
Moved the table to a non-anonymous namespace to make one single instanance that can be shared between the two translation units. I didn't add `inline` as it doesn't have any effect on ODR in an anonymous namespace, so it matches the existing functions in the header.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91140