Page MenuHomePhabricator

[2/N] [libcxx] [test] Add a test for conversions between wchar_t, utf8, char16_t, char32_t and windows native narrow code pages
ClosedPublic

Authored by mstorsjo on Tue, Nov 10, 1:24 AM.

Details

Summary

I don't think the existing path tests really test actual conversion of chars that don't have the same value in all charsets, so this adds test coverage of the path class on posix systems as well.

This is as a separate patch for now, but could also be squashed into one of the later commits that implement the corresponding things for windows.

Diff Detail

Event Timeline

mstorsjo created this revision.Tue, Nov 10, 1:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Nov 10, 1:24 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript

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.

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.

ldionne requested changes to this revision.Tue, Dec 1, 3:35 PM

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.

libcxx/test/std/input.output/filesystems/class.path/path.member/path.charconv.pass.cpp
13

Can you add a comment explaining what this test is about?

27

I would much rather we qualify names instead. fs:: is not much longer, but it adds clarity.

This revision now requires changes to proceed.Tue, Dec 1, 3:35 PM
mstorsjo updated this revision to Diff 308883.Tue, Dec 1, 11:47 PM
mstorsjo edited the summary of this revision. (Show Details)

Added a large descriptive comment at the head of the new test file, removed using namespace fs; so that all uses of path and u8path are qualified into fs::path and fs::u8path.

Oh, and while the test itself works mostly correct on libc++ on posix platforms, the test is written to exercise the new C++20 char8_t input/outputs, so those aspects of the test depends on D90222 (not terribly complex) to make those bits C++20 correct in libc++.

ldionne added inline comments.Thu, Dec 3, 10:34 AM
libcxx/test/std/input.output/filesystems/class.path/path.member/path.charconv.pass.cpp
43

I think this would read better if you used this instead:

#if defined(__GLIBCXX__) && !defined(_WIN32)
#  define HAS_NO_WCHAR
#endif

WDYT?

mclow.lists added inline comments.
libcxx/test/std/input.output/filesystems/class.path/path.member/path.charconv.pass.cpp
89

This should be __cpp_lib_char8_t, because we're using a library facility

336

__cpp_lib_char8_t here too.

mstorsjo added inline comments.Thu, Dec 3, 10:59 AM
libcxx/test/std/input.output/filesystems/class.path/path.member/path.charconv.pass.cpp
43

Sounds good to me, will change it that way

89

Thanks, will change that.

mstorsjo added inline comments.Thu, Dec 3, 11:21 AM
libcxx/test/std/input.output/filesystems/class.path/path.member/path.charconv.pass.cpp
89

I changed the other ifdefs to check __cpp_lib_char8_t as well. While they guard plain char8_t array definitions, they must be in sync with what type path::u8string() returns.

mstorsjo updated this revision to Diff 309322.Thu, Dec 3, 11:23 AM

Updated ifdefs according to suggestions.

ldionne accepted this revision.Thu, Dec 3, 11:32 AM
ldionne set the repository for this revision to rG LLVM Github Monorepo.

LGTM but I'd like to see CI passing.

This revision is now accepted and ready to land.Thu, Dec 3, 11:32 AM