Page MenuHomePhabricator

[8/N] [libcxx] Fix the preexisting directory_iterator code for windows
Needs ReviewPublic

Authored by mstorsjo on Tue, Nov 10, 1:41 AM.

Details

Reviewers
amccarth
EricWF
rnk
Group Reviewers
Restricted Project
Summary

The directory_iterator.cpp file did contain an incomplete, non-working implementation for windows.

Change it to use the wchar version of the APIs.

Don't set the windows specific errors from GetLastError() as code in the generic category; remap the errors to the std::errc values.

Error out cleanly on empty paths.

Invoke FindFirstFile on <directoryname>/* to actually list the entries of the directory.

If the first entry retured by FindFirstFile is to be skipped (e.g. being "." or ".."), call advance() (which calls FindNextFile and loops) which doesn't return until a valid entry is found (or the end is reached).

Diff Detail

Event Timeline

mstorsjo created this revision.Tue, Nov 10, 1:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 10, 1:41 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
rnk resigned from this revision.Tue, Nov 10, 12:25 PM
rnk added a subscriber: amccarth.

Hi, I'm pretty busy right now, and I'm falling behind on other code reviews. Can you send this libc++ patch series to @amccarth? He's on my team, and is familiar with the Win32 API, but is unfamiliar with mingw conventions. Hopefully he has enough experience to help implement these parts of the standard library.

mstorsjo added a subscriber: rnk.Tue, Nov 10, 1:11 PM
In D91140#2386807, @rnk wrote:

Hi, I'm pretty busy right now, and I'm falling behind on other code reviews. Can you send this libc++ patch series to @amccarth? He's on my team, and is familiar with the Win32 API, but is unfamiliar with mingw conventions. Hopefully he has enough experience to help implement these parts of the standard library.

Fair enough, thanks! Luckily, essentially nothing in this patchset is mingw specific (I've tried to steer away from anything that differs, that can be messy). Maybe the only real difference lies in being able to use the __int128 type, see D91139.

amccarth requested changes to this revision.Thu, Nov 12, 10:15 AM

This is pretty close.

libcxx/src/filesystem/directory_iterator.cpp
85–86

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.

libcxx/src/filesystem/filesystem_common.h
23

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?

130

constexpr?

184

Needs inline for ODR, right?

191

inline

This revision now requires changes to proceed.Thu, Nov 12, 10:15 AM

Thanks for having a look!

libcxx/src/filesystem/directory_iterator.cpp
85–86

Good catch, I can try to fix that while fixing up the preexisting code here.

libcxx/src/filesystem/filesystem_common.h
23

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.

130

Will do

184

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.

mstorsjo marked 2 inline comments as done.Fri, Nov 13, 3:31 AM
mstorsjo added inline comments.
libcxx/src/filesystem/filesystem_common.h
23

I removed the need for windows.h in the shared header anyway, without excessive jumping through hoops.

184

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.

mstorsjo updated this revision to Diff 305077.Fri, Nov 13, 3:38 AM
mstorsjo edited the summary of this revision. (Show Details)

Fixed the file size calculation by casting to uint64_t (and doing shifting instead of multiplying by MAXDWORD+1), moved the error mapping table to operations.cpp, removed inclusions of windows.h in the filesystem_common.h header.

amccarth accepted this revision.Wed, Nov 18, 9:35 AM

I'm not a libcxx owner, so I'm not sure what other hoops you should jump through for them, but this looks good to me. Thanks for the improvements!