[libcxx-commits] [PATCH] D151814: [libc++][modules] Adds the C++23 std module.

Sun Jun 4 02:53:44 PDT 2023

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

In D151814#4393479 <https://reviews.llvm.org/D151814#4393479>, @Arthapz wrote:

> do you already know where the modules will be installed (i know modules are not installed atm, it's for simulation purpose :D) ? (like /usr/modules, /usr/module, etc...)

It has already been discussed <https://discourse.llvm.org/t/rfc-about-the-default-location-for-std-modules/69191/48>, but there's no conclusion yet. I want to revisit that discussion soon. That's the main reason not to install modules in this patch.

================
Comment at: libcxx/docs/Modules.rst:7
+
+.. warning:: Modules are an experimental feature. It has additional build
+             requirements and not all libc++ configurations are supported yet.
----------------
ChuanqiXu wrote:
> nit: a silly English question. Is it ok to use `*s` `are` and `an` together in a sentence? I got confused several times before.
I think it's correct, but I'm not a native speaker.

================
Comment at: libcxx/docs/Modules.rst:27
+as BMIs, which are ``.pcm`` files for Clang. BMIs are not portable, they depend
+on the compiler used and its compilation flags. Therefore there needs to be a
+way to distribute the ``.cppm`` files to the user and offer a way for them to
----------------
ChuanqiXu wrote:
> 
I think the current text is correct.

================
Comment at: libcxx/docs/Modules.rst:65
+      `bug report <https://github.com/llvm/llvm-project/issues/61465>`__.
+
+Blockers
----------------
aaronmondal wrote:
> Mordante wrote:
> > aaronmondal wrote:
> > > As a safeguard we might want to add the note that the BMIs shouldn't be distributed at the moment. AFAIK BMIs still contain absolute paths to their build environment. I.e. if you build at `/my/custom/build/machine` this path will be printed if you run `strings` on the BMIs. This could leak information from a distributors build environment to the BMI.
> > I'm not sure that is needed. I feel there are already quite some warnings that BMIs are not portable. At least on Debian in the past, debug packages "leaked" the same information, which didn't seem to be an issue.
> > 
> > Do you have reasons why this is a real issue?
> If you have a build that indexes cached artifacts by hash this can lead to confusing cache behavior for incremental builds. Having the cache poisoned by this essentially forces you to completely delete it and rebuild everything. For LLVM this is roughly 200GB of wasted cache transfer and ~10 hours of wasted single thread CPU minutes. This probably isn't an issue for most users though and since modules are experimental anyways it's probably a non-issue.
That seems like a non-typical use case. I prefer to leave that information out.

================
Comment at: libcxx/docs/Modules.rst:133-134
+  set(CMAKE_CXX_STANDARD_REQUIRED YES)
+  # Clang doesn't support compiler extensions for modules.
+  set(CMAKE_CXX_EXTENSIONS OFF)
+
----------------
Mordante wrote:
> ChuanqiXu wrote:
> > This was mitigated recently: https://github.com/llvm/llvm-project/blob/15a719de01b92da7de4b8381660525b622c2c292/clang/lib/Driver/ToolChains/Clang.cpp#L3688-L3697. But personally I prefer `-std=c++xx` instead of `-std=gnu++xx`.
> Thanks for the info. I too prefer to disable extensions. I'll remove these two lines.
Actually it turns our this doesn't work correctly with libc++. There is an issue with the `new` partition. This is named `__new` since `new` is a keyword. For now I require extensions of and blame libc++ instead of clang.

This should be fixed, but for now I keep it as is and add it to the list of known limitations.

================
Comment at: libcxx/docs/Modules.rst:164
+
+  # Implicit paths are not allowed in the future.
+  add_compile_options($<$<COMPILE_LANGUAGE:CXX>:-fprebuilt-module-path=${CMAKE_BINARY_DIR}/_deps/std-build/CMakeFiles/std.dir/>)
----------------
Mordante wrote:
> ChuanqiXu wrote:
> > This is not what we talked about implicit paths. The implicit paths is that the BMI will contain paths to their dependency implicitly. e.g,
> > 
> > 
> > ```
> > // b.cppm
> > export module b;
> > export int b() {
> >     return 43;
> > }
> > 
> > // a.cppm
> > export module a;
> > import b;
> > export int a() {
> >     return b() + 43;
> > }
> > 
> > // user.cpp
> > import a;
> > int use() {
> >     return a();
> > }
> > ```
> > 
> > Now it is ok to compiler `user.cpp` without specify the path to the BMI of module b. It is different from disabling `-fprebuilt-module-path`.
> Interesting. When I removed the `fprebuilt-module-path` I got these warnings.
I removed the comment and left the compiler flag.

================
Comment at: libcxx/test/libcxx/module_std.sh.cpp:7
+//
+//===----------------------------------------------------------------------===//
+
----------------
Mordante wrote:
> This file needs to be rewritten once D151654 lands.
This file will be removed a `module_std.gen.py` is available. It's still here since the new file is not tested in the CI yet.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151814