[libcxx-commits] [PATCH] D144994: [Draft][libc++][modules] Adds std module.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

In D144994#4282770 <https://reviews.llvm.org/D144994#4282770>, @aaronmondal wrote:

> Started testing the Bazel build. Seems to almost build. The `__new` is a slight inconvenience, but easily worked around and I don't think we can do too much about it.

Yeah we can't use new since it's a keyword.

> I'm also noticing that this will place quite the significant restriction on the compiler building the modules. It's a bit unintuitive to build libc++ with C++20 and the interfaces/partitions with C++23. It's more intuitive to build the entire libc++ (sources) with C++23 (which works), but I'd assume that the only compiler capable of doing that at the moment upstream Clang itself 🤣

We can't nicely build libc++ with C++23, this requires a newer CMake. (I have a patch for it, but I'm waiting for the last buildbots to be updated.)
It could be done with older CMake versions, but that is quite inconvenient.

Since we have a CI it will be trivial to test whether our supported compilers have sufficient C++23 support to build libc++



================
Comment at: libcxx/docs/Modules.rst:83-98
+  * Clang
+
+    * ``__synth_three_way_result`` does not work. There is a work-around in libc++.
+
+      * `bug report <https://github.com/llvm/llvm-project/issues/57222>`__
+      * `clang patch <https://reviews.llvm.org/D140002>`__
+      * `libc++ patch </usr/share/c++/modules/v<x>/`__
----------------
ChuanqiXu wrote:
> Mordante wrote:
> > ChuanqiXu wrote:
> > > Will these problems be blockers actually? Specially for the "header-module" order problem, we can't fix it fundamentally. And I feel 'std module' is still workable in some degree with these defects. If these problems are not blockers actually, could we move the "limitations" section?
> > I think this one isn't I think we should downgrade this one to limitation.
> > 
> What do you mean for `this one`?
This bullet point.


================
Comment at: libcxx/modules/CMakeLists.txt.in:48
+
+add_library(std)
+target_sources(std
----------------
aaronmondal wrote:
> aaronmondal wrote:
> > ChuanqiXu wrote:
> > > Mordante wrote:
> > > > ChuanqiXu wrote:
> > > > > I guess we need a better library name before landing this.
> > > > What would you propose?
> > > > I'm not even sure we really need to, but this is something to determine during a review.
> > > > What would you propose?
> > > 
> > > I use `libstdmodule.a` in the downstream. But it looks not good enough for the upstream.
> > > 
> > > > I'm not even sure we really need to, but this is something to determine during a review.
> > > 
> > > Ideally this should be handled by cmake (or other build systems). But we need it now to make progress. How about something like 'libc++_std_module.a'?
> > Don't have a strong opinion on this, though this will probably be painful to change once users adopt any name.
> > 
> > `libc++-std.a`, `lib++_std.a`? Though I personally think just `std` would be fine.
> > 
> > The `std.compat` archive should follow a similar scheme though.
> I think I'm misunderstanding this target. What are the contents of this archive?
This is the library for the module std. So next to the `.pcm` files there is a library component.


================
Comment at: libcxx/modules/std/memory.cppm:183-184
+
+  // [util.smartptr.atomic], atomic smart pointers
+  using std::atomic;
+
----------------
aaronmondal wrote:
> This seems to break the build for me with "no member atomic in namespace std". Where is this symbol coming from?
> 
> The `<atomic>` include is disabled for C++23. Maybe I'm just missing some config flag?
I'm not sure, but there are several errors in the modules. I copy-paste the synopis and manually cleaned them, so errors were always bound to occur.

There is a test `module_std.sh.cpp` which validates the declarations in the include files and in the modules.
This found quite some issues, some parts are not implemented in libc++, the synopsis only sometimes doesn't have all declarations (bitmask types for example), etc/
I'm not sure whether the test flags this one, but I have some patches to fix some of the issue found in libc++ itself. 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144994