[libcxx-commits] [PATCH] D91133: [2/N] [libcxx] [test] Add a test for conversions between wchar_t, utf8, char16_t, char32_t and windows native narrow code pages

[Louis Dionne via Phabricator via libcxx-commits]

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

In D91133#2426694 <https://reviews.llvm.org/D91133#2426694>, @mstorsjo wrote:

> In D91133#2426605 <https://reviews.llvm.org/D91133#2426605>, @amccarth wrote:
>
>> These test look pretty thorough.  I think these will be useful in finding portability problems.
>>
>> Given the TODO in the description, I'll hold off on accepting this revision.
>
> @ldionne Where should this kind of new test be placed? It doesn't test strictly one specific method, but tests some specific aspects of both constructors and observers.

I think `libcxx/test/std/input.output/filesystems/class.path/path.member/path.charconv.pass.cpp`, as you did, is fine.



================
Comment at: libcxx/test/std/input.output/filesystems/class.path/path.member/path.charconv.pass.cpp:12
+
+// <filesystem>
+
----------------
Can you add a comment explaining what this test is about?


================
Comment at: libcxx/test/std/input.output/filesystems/class.path/path.member/path.charconv.pass.cpp:26
+
+using namespace fs;
+
----------------
I would much rather we qualify names instead. `fs::` is not much longer, but it adds clarity.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91133