[libcxx-commits] [PATCH] D98960: [libcxx] [test] Allow C:\System Volume Information to be missing

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/test/std/input.output/filesystems/fs.op.funcs/fs.op.exists/exists.pass.cpp:82
+    if (p.empty())
+        TEST_UNSUPPORTED();
 #else
----------------
Quuxplusone wrote:
> What happens if you call `TEST_UNSUPPORTED` from //within// `GetWindowsInaccessibleDir`?
> 
> The current code smells very bad to me — the caller unsupports the test, but the callee fprintfs the warning message about how the test is going to be unsupported. These two behaviors should happen in the same place one way or another.
> 
> Have you already investigated and rejected whether there's a way to //reliably create// an inaccessible directory for testing, instead of relying on this pre-existing directory?
Yes we've had multiple persons look into options for what to use for inaccessible directory tests already in an earlier review thread. It's not entirely trivial.

You're not really supposed to call any of the `TEST_*` macros from within the support header. The `TEST_*` macros expand to a bit code that assumes that it's directly within the `TEST_CASE() {}` block. And not all callers use such a setup (as opposed to just a plain `main()` function with asserts). The `TEST_UNSUPPORTED()` macro just does some bookkeeping and then calls `return;` and, and a plain `return;` to stop the execution of the current test doesn't work from nested functions.

Yes it's not entirely pretty, but this way, the function explains exactly what/why was amiss regarding the test directory, regardless of what the caller will do with the information, and the caller will either mark the whole test as unsupported, or skip parts of it. E.g. in D98443, in temp_directory_path.pass.cpp, the right course of action is just to skip a small bit of the test, not bail out of the whole test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98960