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

[Chuanqi Xu via Phabricator via libcxx-commits]

ChuanqiXu added a comment.

Thanks! This looks good overall.

> There is no test that libc++ can be fully imported as a module.

What's the plan to add some test to use  `import std;` in the source code? I remember you had a patch to use `import std;` for the coroutines tests. But I can't find it now. For me, it is better to have at least one `import std;` in the test of the tree. (But this is not necessary to land it in this patch, of course)



================
Comment at: libcxx/docs/Contributing.rst:38
 - Did you mark all functions and type declarations with the :ref:`proper visibility macro <visibility-macros>`?
+- Did you add all new named declarations to the ``std`` module?
 - If you added a header:
----------------
nit: in my imagination, in the future, this can be solved automatically:

```
#if ...
import std;
#else
#include <stl_headers>
#endif

use of new decl... // error if the author forget to touch std modules.
```

(this is not a requirement to change the note)


================
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.
----------------
nit: a silly English question. Is it ok to use `*s` `are` and `an` together in a sentence? I got confused several times before.


================
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
----------------



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


================
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/>)
----------------
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`.


================
Comment at: libcxx/docs/ReleaseNotes.rst:147
 
+- The lit test parameter ``enable_modules`` will no longer support ``True`` and ``False`` as valid input.
+
----------------
nit: maybe it is helpful to mention the above change. Otherwise the readers who don't read the whole document may get confused.


================
Comment at: libcxx/modules/CMakeLists.txt:2
+if (CMAKE_VERSION VERSION_LESS 3.26)
+  message(WARNING "The libc++ modules won't be available because the version of CMake is too old to support them.")
+  return()
----------------
nit: consider to specify 3.26 clearly in the message?


================
Comment at: libcxx/modules/CMakeLists.txt.in:1
+# CMake 3.25 has modules support, but that does not work properly.
+cmake_minimum_required(VERSION 3.26)
----------------
nit: CMake3.25 only has the support MSVC and the support for clang was added in 3.26.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151814