This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Fix temp_directory_path for windows
ClosedPublic

Authored by mstorsjo on Mar 7 2021, 2:00 AM.

Details

Summary

Check a different set of env vars, don't check the exact value
of the fallback path. (GetTempPath falls back to returning c:\windows
if nothing better is available in env vars.)

Diff Detail

Event Timeline

mstorsjo requested review of this revision.Mar 7 2021, 2:00 AM
mstorsjo created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 7 2021, 2:00 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
curdeius accepted this revision as: curdeius.Mar 8 2021, 12:06 PM
curdeius added a subscriber: curdeius.

LGTM if you clarify the comment.

libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.temp_dir_path/temp_directory_path.pass.cpp
122

Nit: capital in Windows.

122

I'd rather go for a precise piece of information in the comment, as you wrote in the revision summary, the fallback is the Windows folder (https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-gettemppatha#remarks).

mstorsjo updated this revision to Diff 329095.Mar 8 2021, 12:10 PM

Clarified the comment as requested

curdeius added inline comments.Mar 8 2021, 1:04 PM
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.temp_dir_path/temp_directory_path.pass.cpp
122

Nit: the Windows folder is not necessarily C:\Windows. Don't bother to change it before landing.

mstorsjo added inline comments.Mar 8 2021, 1:07 PM
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.temp_dir_path/temp_directory_path.pass.cpp
122

Oh, right, that's true, thanks!

Ping for second approval

Ping for second approval

Quuxplusone added inline comments.
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.temp_dir_path/temp_directory_path.pass.cpp
67

Is there any significance to starting with dir2 on Windows versus dir1 on POSIX?

Also, this is scope creep, but do you maybe want to (in a separate PR) make a test case to show that the setting of env var TMPDIR is correctly ignored on Windows?

mstorsjo added inline comments.Mar 15 2021, 9:49 AM
libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.temp_dir_path/temp_directory_path.pass.cpp
67

Not really, just copy/paste laziness, I can change it to start with dir1 there too.

Hmm, I guess I could try to extend the test to check for ignored variables too, as a later patch.

Quuxplusone accepted this revision.Mar 15 2021, 9:55 AM

LGTM after changing to dir1 etc.; please double-check that the tests still pass after that change, then ship it!

This revision is now accepted and ready to land.Mar 15 2021, 9:55 AM