Details
- Reviewers
ldionne ChuanqiXu aaronmondal jdoerfert - Group Reviewers
Restricted Project - Commits
- rGc40595f2ce29: [libc++][modules] Adds std module cppm files.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libcxx/modules/std/new.cppm | ||
---|---|---|
13 | 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 😅 |
libcxx/modules/std/new.cppm | ||
---|---|---|
13 | 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. |
libcxx/modules/std/new.cppm | ||
---|---|---|
13 | 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 | ||
---|---|---|
20 | It looks better if we can use meaningful name like #ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS. |
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 | ||
---|---|---|
13 | 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 | ||
20 | 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.) |
Got it. So this looks good to me : )
libcxx/modules/std/vector.cppm | ||
---|---|---|
20 | Yeah, it should be up to you. |
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!
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 😅