This implements the std::filesystem parts of P0482 (which is already marked as in progress), and applies the actions that are suggested in P1423.
Details
- Reviewers
EricWF curdeius ldionne - Group Reviewers
Restricted Project - Commits
- rG6be11e35d539: [libcxx] Implement c++2a char8_t input/output of std::filesystem::path
Diff Detail
Event Timeline
Some questions:
- Which part of which paper is this implementing?
- Do we need to update the cxx2a_status page?
- Is there a feature test macro associated to this?
This implements bits of P0482; some parts of it seem to be implemented already, and this completes the filesystem related bits, but afaik there's other bits of it left unimplemented as well. It also takes some suggestions from P1423 into account. I guess the cxx2a_status page already could be updated to mark at least P0482 as in progress as things are already.
As far as I can see, that paper adds the feature macro __cpp_lib_char8_t which already is defined, since D55308.
I made this patch the start of the patch series implementing filesystem for windows; as character sets are handled a bit differently on windows (the narrow charset isn't necessarily utf8), and this patch adds new ways of interacting with utf8 in filesystem, I wanted to have this in place first, to get those aspects right from the start - but if needed, the windows patches can be rebased to not rely on this one.
It looks pretty good for me, just minor comments.
And please put the relevant papers in the patch description, so that they appear in the commit later.
libcxx/include/filesystem | ||
---|---|---|
1236 | Unnecessary comment? You should maybe mark P1483 as In Progress. | |
1241 | You should update the message too. | |
1251–1255 | Ditto. Comment about P1423 and message. | |
libcxx/test/support/filesystem_test_helper.h | ||
433 | Shouldn't you guard it on __cpp_char8_t instead of using dummy below? |
libcxx/include/filesystem | ||
---|---|---|
1236 | Sure | |
1241 | Oh, good catch, thanks! | |
libcxx/test/support/filesystem_test_helper.h | ||
433 | Yeah, it's possible, with something like this: #if TEST_STD_VER > 17 && defined(__cpp_char8_t) #define CHAR8_ONLY(x) x, #else #define CHAR8_ONLY #endif #define MKSTR(Str) {... CHAR8_ONLY(TEST_CONCAT(u8, Str)) TEST_CONCAT(u, Str), ... The trailing comma is a bit tricky; it has to be included as part of the expansion of CHAR8_ONLY(), but this way it seems like it would work. If you prefer that form, I can change it that way. |
Updated the status of P1423, removed unnecessary comments, added optional references to char8_t in static assert messages, removed a dummy variable in the pre-C++20 version of the test helper, updated the commit message to include references to both papers.
LGTM. But wait for an approval from a higher instance :).
libcxx/test/support/filesystem_test_helper.h | ||
---|---|---|
433 | I thought of just doing: #if TEST_STD_VER... #define MKSTR(...u8...) #else #define MKSTR(...) #endif But that's OK as it is for me. Macros are tricky (or evil). |
libcxx/include/filesystem | ||
---|---|---|
1005 | That's an ABI break in C++20. Do we think it's reasonable? I think we at least need to add it to the release notes. |
libcxx/include/filesystem | ||
---|---|---|
1005 | This method is marked with the _LIBCPP_INLINE_VISIBILITY attribute, which afaik is supposed to exclude it from the ABI altogether - exactly for this reason? The fact that it's a source compat break, and whether to worry about it or not, is acknowledged and discussed in P1423. |
libcxx/include/filesystem | ||
---|---|---|
1005 | I was trying to come up with examples where a library was already compiled with an inlined version of path::u8string that returns a std::string, and then link against another binary where path::u8string() returns a std::u8string instead. But I couldn't find something that wasn't very tortuous, so I think this is fine from the ABI perspective. Can we please add a release note because this is a source break? |
That's an ABI break in C++20. Do we think it's reasonable?
I think we at least need to add it to the release notes.