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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked 2 inline comments as done.
Mordante added inline comments.


================
Comment at: libcxx/stdmodules/CMakeLists.txt:20
+set(CMAKE_EXPERIMENTAL_CXX_MODULE_MAP_FORMAT "clang")
+set(CMAKE_EXPERIMENTAL_CXX_MODULE_MAP_FLAG "@<MODULE_MAP_FILE>")
+
----------------
aaronmondal wrote:
> AFAIK modulemaps are not supported for standard modules.
> 
> If this is so that we can test both the clang-specific and the standard variants we should probably put a comment here to avoid confusion.
This is what CMake does too. I assume this hunk will be shipped with CMake so for now I keep this as is and maybe I even remove it when this will be shipped in CMake 3.26.


================
Comment at: libcxx/stdmodules/CMakeLists.txt:43
+    -stdlib=libc++
+    -fprebuilt-module-path=${PREBUILT_MODULE_PATH}
+    -pthread
----------------
aaronmondal wrote:
> aaronmondal wrote:
> > I remember running into issues with `-fprebuilt-module-path` as it assumes a very specific output layout during the build. For build systems that sandbox compilations the (absolute) path to the output file (and to the headers it included?) will be embedded in the PCM and aggregating all BMIs in one directory after precompilation might lead to "dependency not found"-type errors.
> > 
> > Explicitly passing the files via `-fmodule-file=<path/to/BMI>` or `-fmodule-file=<module_name>=<path/to/BMI>` seems more robust to me as this takes the situation into account where BMI outputs are potentially temporarily scattered across directories.
> > 
> > I think for CMake the current `-fprebuilt-module-path` should work fine. I'm not sure about gn, and I know for sure that something like this won't work in Bazel.
> Amend: `-fmodule-file=<path/to/BMI>` will probably be deprecated (https://github.com/llvm/llvm-project/issues/60824), so something like `-fmodule-file=std:header=<path/to/header>` seems like the way to go.
I agree. This was mainly to show the difference between manually crafting pcm files vs letting cmake doing it. This was the work I did last weekend.

After yesterday's meeting I think this entire approach in CMake is flawed, this approach assumes we're going to distribute the `.pcm` files. Instead we need to focus on distributing `.cppm` files and offer the tools for the user to create the `.pcm` files for their project. This is something I will investigate further.


================
Comment at: libcxx/stdmodules/std.cppm:7
+export import :vector;
+export import :exception;
----------------
aaronmondal wrote:
> Ah, so pretty ❤️ this should make it possible to build all the partitions in parallel and I think this is about as incremental-build-friendly as we can get.
Not only that is a benefit. With my maintainers hat on I feel managing several smaller files is a lot easier than one big file. Especially since we want to add additional comments in these files.


================
Comment at: libcxx/utils/libcxx/test/params.py:110-111
               AddFeature('modules-build'),
               AddCompileFlag('-fmodules'),
               AddCompileFlag('-fcxx-modules'), # AppleClang disregards -fmodules entirely when compiling C++. This enables modules for C++.
+            ] if enable_modules == "clang" or enable_modules == "True" else [
----------------
aaronmondal wrote:
> Is this for standard or non-standard modules? AFAIK the `-std=c++20` flag should already enable (standard) modules so there may be no need to have `-fmodules` here at all.
> 
> We might be missing docs here as neither https://clang.llvm.org/docs/Modules.html nor https://clang.llvm.org/docs/StandardCPlusPlusModules.html mention `-fcxx-modules` and the `clang --help` output also doesn't clarify this.
> 
> There is also a `-xc++-module` flag which tells clang to treat the input file as c++ standard module interface regardless of file extension (although I'm quite in favor of that `.cppm` extension 😄)
Libc++ already supports Clang's module extension. That's why the target is called `clang`. This has nothing to do with the Standard modules. The part starting at line 113 is what's needed for C++ modules. Who doesn't love trailing if's ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144994