Page MenuHomePhabricator

[1/N] [libcxx] Implement c++2a char8_t input/output of std::filesystem::path
ClosedPublic

Authored by mstorsjo on Oct 27 2020, 4:41 AM.

Details

Summary

This implements the std::filesystem parts of P0482 (which is already marked as in progress), and applies the actions that are suggested in P1423.

Diff Detail

Event Timeline

mstorsjo created this revision.Oct 27 2020, 4:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2020, 4:41 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
mstorsjo requested review of this revision.Oct 27 2020, 4:41 AM

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?

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.

mstorsjo retitled this revision from [libcxx] Implement c++2a char8_t input/output of std::filesystem::path to [1/N] [libcxx] Implement c++2a char8_t input/output of std::filesystem::path.

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.

mstorsjo updated this revision to Diff 308564.Tue, Dec 1, 12:36 AM

Rebased past conflicts in include/__config.

mstorsjo updated this revision to Diff 308892.Wed, Dec 2, 12:17 AM

Rebased on top of D92250.

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.

1240–1244

You should update the message too.

1254–1258

Ditto. Comment about P1423 and message.

libcxx/test/support/filesystem_test_helper.h
433–439

Shouldn't you guard it on __cpp_char8_t instead of using dummy below?

mstorsjo added inline comments.Wed, Dec 2, 3:21 AM
libcxx/include/filesystem
1236

Sure

1240–1244

Oh, good catch, thanks!

libcxx/test/support/filesystem_test_helper.h
433–439

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.

mstorsjo updated this revision to Diff 308929.Wed, Dec 2, 3:49 AM
mstorsjo edited the summary of this revision. (Show Details)

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.

curdeius accepted this revision.Wed, Dec 2, 4:27 AM

LGTM. But wait for an approval from a higher instance :).

libcxx/test/support/filesystem_test_helper.h
433–439

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).

ldionne requested changes to this revision.Thu, Dec 3, 8:30 AM
ldionne set the repository for this revision to rG LLVM Github Monorepo.
ldionne added inline comments.
libcxx/include/filesystem
1006

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.

This revision now requires changes to proceed.Thu, Dec 3, 8:30 AM
mstorsjo added inline comments.Thu, Dec 3, 8:35 AM
libcxx/include/filesystem
1006

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.

ldionne added inline comments.Thu, Dec 3, 8:53 AM
libcxx/include/filesystem
1006

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?

mstorsjo updated this revision to Diff 309291.Thu, Dec 3, 9:53 AM

Added a release note about source incompatibility.

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