[libcxx-commits] [PATCH] D151030: [libc++][modules] Adds std module cppm files.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante added a subscriber: H-G-Hristov.
Mordante added a comment.

In D151030#4359443 <https://reviews.llvm.org/D151030#4359443>, @ChuanqiXu wrote:

> 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 <https://reviews.llvm.org/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.



================
Comment at: libcxx/modules/std/new.cppm:13
+
+export module std:__new; // Note new is a keyword and not a valid identifier
+export namespace std {
----------------
aaronmondal wrote:
> Mordante wrote:
> > aaronmondal wrote:
> > > 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 😅
> > 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.
> 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",
>     },
> ```
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.)


================
Comment at: libcxx/modules/std/vector.cppm:20
+  using std::operator==;
+#if 0 // P1614
+  using std::operator<=>;
----------------
ChuanqiXu wrote:
> It looks better if we can use meaningful name like `#ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS`. 
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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151030/new/

https://reviews.llvm.org/D151030