This is an archive of the discontinued LLVM Phabricator instance.

[libc++][modules] Adds std module cppm files.
ClosedPublic

Authored by Mordante on May 20 2023, 7:58 AM.

Details

Summary

This adds the cppm files of D144994. These files by themselves will do
nothing. The goal is to reduce the size of D144994 and making it easier
to review the real changes of the patch.

Implements parts of

  • P2465R3 Standard Library Modules std and std.compat

Diff Detail

Event Timeline

Mordante created this revision.May 20 2023, 7:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2023, 7:58 AM
Mordante requested review of this revision.May 20 2023, 7:58 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
aaronmondal added inline comments.May 20 2023, 8:56 AM
libcxx/modules/std/new.cppm
14

I wonder whether it would be better to name this file __new.cppm, so that it's more straightforward to generate the module partition name from the filename. Otherwise this needs special treatment in build files.

Then again, that would make the filename inconsistent with the others. Ahh what a dilemma 😅

Mordante added inline comments.May 20 2023, 12:13 PM
libcxx/modules/std/new.cppm
14

For CMake I have a hard-coded list where I can use __new.cppm or new.cppm. (That CMake file's not part of this review since this review should be about adding these files and no logic.)

When this makes it easier for you I've no objection against renaming this file.

aaronmondal added inline comments.May 20 2023, 12:36 PM
libcxx/modules/std/new.cppm
14

Hmm thinking about this some more I think renaming would be better. It's not exactly making integration harder, but it does add some avoidable complexity:

interfaces = {
    "modules/std/{}.cppm".format(name): "std:{}".format(name)
    for name in STD_PARTITIONS
} | {
    "modules/std/new.cppm": "std:__new",
},

I just took a look at it and it looks almost fine. I can't be sure if there is anything missing. But it may be better to decide this by testing instead of by eyes. So this looks good to me.

libcxx/modules/std/vector.cppm
21

It looks better if we can use meaningful name like #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS.

Mordante marked an inline comment as done.May 22 2023, 9:29 AM
Mordante added a subscriber: H-G-Hristov.

I just took a look at it and it looks almost fine. I can't be sure if there is anything missing. But it may be better to decide this by testing instead of by eyes. So this looks good to me.

This has indeed been tested by a test script in D144994. That script has found quite some errors I made. It also helped a lot to update the .cppm files after rebasing. It now matches the current implementation status of libc++. Once the rest of the infrastructure lands this script will make sure the CI fails when the headers and the .cppm are out of sync.

libcxx/modules/std/new.cppm
14

Unfortunately I noticed there is an issue with renaming. I have a script that tests whether module foo matches header foo. That test would break with the rename.

Just curious do you really need to use the partitions directly. The Standard doesn't mandate any partitions, I could have dumped all declarations in one big .cppm file and be compliant. (Obviously that would be a maintenance nightmare.)

libcxx/modules/std/vector.cppm
21

I expect these to be short-lived @H-G-Hristov is putting quite some effort in implementing P1614. The #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS is intended to be forever. I've done similar things for other missing parts of headers or entire headers. For example print.cppm exists but libc++ doesn't have <print> yet. (That header is being worked on but it hasn't been reviewed yet.)

ChuanqiXu accepted this revision.May 22 2023, 6:56 PM

I just took a look at it and it looks almost fine. I can't be sure if there is anything missing. But it may be better to decide this by testing instead of by eyes. So this looks good to me.

This has indeed been tested by a test script in D144994. That script has found quite some errors I made. It also helped a lot to update the .cppm files after rebasing. It now matches the current implementation status of libc++. Once the rest of the infrastructure lands this script will make sure the CI fails when the headers and the .cppm are out of sync.

Got it. So this looks good to me : )

libcxx/modules/std/vector.cppm
21

Yeah, it should be up to you.

aaronmondal accepted this revision.May 22 2023, 7:20 PM

Tested this against my downstream build files. Builds and runs on my end (with the tuple fix from D144994).

libcxx/modules/std/new.cppm
14

No worries then. That maintenance burden sounds much worse than that slight inconvenience I mentioned.

ldionne accepted this revision.May 23 2023, 9:25 AM

This LGTM but I'd like to check in the test that pins down this content reasonably quickly after to make sure this doesn't rot. Thanks!

This revision is now accepted and ready to land.May 23 2023, 9:25 AM
Mordante marked 5 inline comments as done.May 23 2023, 9:50 AM
This revision was landed with ongoing or failed builds.May 23 2023, 9:51 AM
This revision was automatically updated to reflect the committed changes.