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

[Aaron Siddhartha Mondal via Phabricator via libcxx-commits]

aaronmondal accepted this revision.
aaronmondal added a comment.

Tested this against my downstream build files. Builds and runs on my end (with the tuple fix from D144994 <https://reviews.llvm.org/D144994>).



================
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 {
----------------
Mordante wrote:
> 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.)
No worries then. That maintenance burden sounds much worse than that slight inconvenience I mentioned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151030