[libcxx-commits] [PATCH] D98166: [libcxx] Test accessing a directory on windows that gives "access denied" errors

[Marek Kurdej via Phabricator via libcxx-commits]

curdeius requested changes to this revision.
curdeius added a comment.
This revision now requires changes to proceed.

Almost there. :)



================
Comment at: libcxx/test/std/input.output/filesystems/class.directory_entry/directory_entry.cons/path.pass.cpp:152
+#ifndef _WIN32
+  // Windows doesn't support setting perms::none to trigger failures
   scoped_test_env env;
----------------
Nit: full stops at end of comments. And it seems that this comment is just truncated.


================
Comment at: libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.exists/exists.pass.cpp:76
 {
+#ifndef _WIN32
+    // Windows doesn't support setting perms::none to trigger failures
----------------
For the ease of reading, please swap the branches (here and in other tests):
```
#ifdef _WIN32
// current else
#else
// current if
#endif
```


================
Comment at: libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.exists/exists.pass.cpp:84
+#else
+    // Actually a directory, not a file
+    const path file("C:\\System Volume Information");
----------------
mstorsjo wrote:
> curdeius wrote:
> > FYI. If you really need a file instead of a directory, there are system files in C:\ (usually, but they can be moved): `hiberfil.sys`, `pagefile.sys`, `swapfile.sys`.
> Yeah. For this case it doesn't matter if it's a file or directory, but perhaps it's clearer to rename file to path here.
Yep, that's good!


================
Comment at: libcxx/test/support/filesystem_test_helper.h:668
 
+inline fs::path GetWindowsInaccesibleDir() {
+  // Only makes sense on windows, but the code can be compiled for
----------------
Typo: `Inaccessible` (double 's').


================
Comment at: libcxx/test/support/filesystem_test_helper.h:668
 
+inline fs::path GetWindowsInaccesibleDir() {
+  // Only makes sense on windows, but the code can be compiled for
----------------
curdeius wrote:
> Typo: `Inaccessible` (double 's').
Nit: why not snake_case? The same for `endIt`.
Or actually. Never mind, we have `GetTestEC` too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98166