Page MenuHomePhabricator

[17/N] [libcxx] Implement the read_symlink function for windows
Needs ReviewPublic

Authored by mstorsjo on Tue, Nov 10, 8:40 AM.

Details

Reviewers
amccarth
EricWF
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

mstorsjo created this revision.Tue, Nov 10, 8:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 10, 8:40 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
compnerd added inline comments.
libcxx/src/filesystem/operations.cpp
1497

You should use backup semantics here to ensure that directories can be read.

FWIW, after this patch, it should compile successfully for windows, the later patches only fix remaining behaviours.

libcxx/src/filesystem/operations.cpp
1497

This does use backup semantics; see https://reviews.llvm.org/D91141 for the implementation of the WinHandle helper. The flag FILE_FLAG_BACKUP_SEMANTICS is always included, to keep the calling code terse here.

mstorsjo updated this revision to Diff 305085.Fri, Nov 13, 3:47 AM
mstorsjo added a reviewer: amccarth.

Rebased on top of updated patches.

This looks like it'll work just fine, but I've added some inline questions for readability and robustness.

libcxx/src/filesystem/operations.cpp
1495

What's the purpose of the + sizeof(wchar_t) here?

1505

This seems like a reasonable time to use const auto *.

1507

I'm wondering whether it's worth doing some bounds checking on out and the offsets and lengths into PathBuffer. Is the data from DeviceIoControl trustworthy? Could somebody create a bad reparse point that would cause this code to access arbitrary data?

I think the answer is probably not.

Does libcxx have a "debug" version with assertions enabled?

1508

const auto &?