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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 5 inline comments as done.
Mordante added a comment.

Thanks for the comments!

In D144994#4164078 <https://reviews.llvm.org/D144994#4164078>, @ChuanqiXu wrote:

> BTW, I am also curious that if the current phab CI runs the std_modules test now? If yes, it is pretty good.

The current patch will not run in the CI. I want to update the CI to use CMake 3.26 after it has been released and it needs Ninja 1.11 too. However the patch works locally for me for the modified tests. So I expect little trouble with the CI after it has been updated. (I intend to test locally with our CI Docker file with a CMake 3.26-rcX to make sure it works.)



================
Comment at: libcxx/CMakeLists.txt:422
   set(LIBCXX_GENERATED_INCLUDE_DIR "${LLVM_BINARY_DIR}/include/c++/v1")
+  set(LIBCXX_GENERATED_MODULE_DIR "${LLVM_BINARY_DIR}/include/c++/modules")
   set(LIBCXX_GENERATED_INCLUDE_TARGET_DIR "${LLVM_BINARY_DIR}/include/${LLVM_DEFAULT_TARGET_TRIPLE}/c++/v1")
----------------
aaronmondal wrote:
> This seems like a good opportunity to finally give `v1` a friend 😊
> 
> Is there some resource on why/how the `v1` path came to be in the first place?
> 
> I'd expect this to {break/be incompatible with} quite a few {distros/setups/build sytems} as the path to `v1` is probably one of the most often hardcoded paths in history 😅.
> 
> Whatever the final name of this `modules` directory is, it would probably be a good idea to have some kind of "reasoning" behind it so that adopters (and other c++ standard libraries?) don't feel like this name was chosen arbitrarily.
ABI v1.

I actually think the current location is horrible ;-) As documented and commented below the final location needs a bit more thought. However for testing it now it's "good enough". So I fully share your concerns, but for testing I need *a* location. For the real product we need to consider *the proper* location.




================
Comment at: libcxx/docs/Modules.rst:65
+   to discuss that approach before spending effort in it.
+ * Currently the ``cppm`` file and the generated ``CMakeLists.txt`` are not
+   installed. Before doing that it would be good to determine what the best
----------------
aaronmondal wrote:
> We intend to distribute all `.cppm` files, i.e. the partitions as well, right?
Yes I want to distribute them, in fact we must distribute them else users can't use modules.

But we need to find the "proper" location to store them. Since this is something new this probably needs some involvement of our packagers. I don't expect the Linux Standard Base has an entry for C++20 modules.


================
Comment at: libcxx/stdmodules/CMakeLists.txt:12-21
+set(_all_modules "${LIBCXX_GENERATED_MODULE_DIR}/CMakeLists.txt")
+foreach(f ${LIBCXX_MODULE_SOURCES})
+  set(src "${CMAKE_CURRENT_SOURCE_DIR}/${f}")
+  set(dst "${LIBCXX_GENERATED_MODULE_DIR}/${f}")
+  add_custom_command(OUTPUT ${dst}
+    DEPENDS ${src}
+    COMMAND ${CMAKE_COMMAND} -E copy_if_different ${src} ${dst}
----------------
ChuanqiXu wrote:
> What's the intention of this code? From my understanding, we don't need to copy the .pcm files when cmake can handle them.
This copies the `cppm` files, which is also not strictly needed. However libc++ creates a installation like structure in the build directory before invoking the tests. We do the same for the normal header files. So I wanted to keep this code in the same fashion. I feel this makes it easier to maintain this code in libc++.


================
Comment at: libcxx/test/std_modules/language.support/support.coroutines/coroutine.handle/coroutine.handle.capacity/operator_bool.pass.cpp:19-24
+#ifdef TEST_MODULES
+import std;
+#else
+#  include <coroutine>
+#  include <type_traits>
+#endif
----------------
ChuanqiXu wrote:
> philnik wrote:
> > Mordante wrote:
> > > philnik wrote:
> > > > Couldn't we just have a header with `import std;` in it that is always included in the C++ modules build? We have to test `#include`ing the headers and modules together anyways. That would avoid touching every single test and adding a preprocessor conditional.
> > > Interesting point. I think we then need 3 versions:
> > > - module with only an import
> > > - module with import and test whether works with includes
> > > - non-module only using includes.
> > > 
> > > But this is indeed one of the things we need to think about how we want to tackle it.
> > For "only an import" it would be possible to have dummy headers, which just export nothing. It's probably technically not standards-conforming, but I don't think that's a real problem.
> While I don't maintain libcxx, I suggest to use the current style. Since I feel it has better readability.
I rather not use code that's not strictly Standard conforming. Both MSVC STL and libstdc++ use our test suite, so violating the Standard may work for us, but it may break them.


================
Comment at: libcxx/utils/libcxx/test/params.py:116
+              AddCompileFlag('-DTEST_MODULES'),
+              AddCompileFlag(lambda cfg: '-fprebuilt-module-path=' + os.path.join(cfg.test_exec_root, '__config_module__/CMakeFiles/std.dir')),
+              AddLinkFlag(lambda cfg: os.path.join(cfg.test_exec_root, '__config_module__/libstd.a')),
----------------
ChuanqiXu wrote:
> Do we still need to pass `-fprebuilt-module-path=` now? From my understanding, cmake should manage the dependencies between modules.
Yes, we do. This is part of the lit framework. This doesn't use CMake, instead it invokes clang++ directly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144994