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

[Aaron Siddhartha Mondal via Phabricator via libcxx-commits]

aaronmondal added a comment.

I really like this so far. I think we need to make sure though that we don't confuse standard and non-standard modules though. If we want to test both, we should clearly label them, though it's probably a good idea to only use the intended-to-be-standard-conform variant for this prototype patch.

If I understood @ChuanqiXu correctly we are generating very similar AST dumps for both standard and non-standard precompilation, but there are some subtle differences between the two and as standard modules mature they will likely diverge further from the old-style PCMs in the (very far 😄) future.



================
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>")
+
----------------
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.


================
Comment at: libcxx/stdmodules/CMakeLists.txt:43
+    -stdlib=libc++
+    -fprebuilt-module-path=${PREBUILT_MODULE_PATH}
+    -pthread
----------------
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.


================
Comment at: libcxx/stdmodules/std.cppm:7
+export import :vector;
+export import :exception;
----------------
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.


================
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 [
----------------
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 😄)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144994