This is an archive of the discontinued LLVM Phabricator instance.

[19/N] [libcxx] Implement temp_directory_path using GetTempPath on windows
ClosedPublic

Authored by mstorsjo on Nov 10 2020, 8:43 AM.

Details

Summary

This does roughly the same as the manual implementation, but checks a slightly different set of environment variables and has a more appropriate fallback if no environment variables are available (/tmp isn't a very useful fallback on windows).

Diff Detail

Event Timeline

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

What do you think of using GetTempPath instead? That is more likely to be writable.

mstorsjo added inline comments.Nov 10 2020, 10:57 AM
libcxx/src/filesystem/operations.cpp
1599

That's also an option - or we could skip our own code and just use that one. That function does essentially the same, checks a few env vars (two of the ones checked here, plus USERPROFILE). But if none of the env vars are available, GetTempPath also falls back to c:\windows - that's where I got the idea.

amccarth requested changes to this revision.Dec 7 2020, 2:57 PM
amccarth added inline comments.
libcxx/src/filesystem/operations.cpp
1599

My first choice would be to use GetTempPathW directly, since that will match expectations for Windows programmers and be relatively future proff.

My second choice would be to keep the custom iteration through the well-known environment variables but to replace the hardcoded c:\windows with a call to GetWindowsDirectoryW.

This revision now requires changes to proceed.Dec 7 2020, 2:57 PM
mstorsjo updated this revision to Diff 310107.Dec 8 2020, 1:37 AM
mstorsjo retitled this revision from [19/N] [libcxx] Fix the fallback case in temp_directory_path for windows to [19/N] [libcxx] Implement temp_directory_path using GetTempPath on windows.
mstorsjo edited the summary of this revision. (Show Details)

Using GetTempPathW instead of the manual code checking environment variables.

amccarth accepted this revision.Dec 8 2020, 1:56 PM

LGTM.

libcxx/src/filesystem/operations.cpp
1702

My gut said >= rather than >, but the documentation for this API is a cagey about this boundary condition. I experimented and realized this does work with > because there is no case where it would return exactly the buffer size.

mstorsjo added inline comments.Dec 8 2020, 2:07 PM
libcxx/src/filesystem/operations.cpp
1702

Yeah, the return value handling there is pretty tricky - GetFinalPathNameByHandleW had the same pattern.

ldionne set the repository for this revision to rG LLVM Github Monorepo.Jan 28 2021, 3:19 PM
mstorsjo updated this revision to Diff 320108.Jan 29 2021, 5:06 AM
mstorsjo removed rG LLVM Github Monorepo as the repository for this revision.
mstorsjo set the repository for this revision to rG LLVM Github Monorepo.

Rebased, retriggering CI.

For this function, there's not much of a corresponding posix-like function to hide the functionality within.

ldionne accepted this revision.Feb 5 2021, 11:44 AM
This revision is now accepted and ready to land.Feb 5 2021, 11:44 AM