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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

This pretty much looks good to me. I'd like to see it one last time after applying the comments but I think we can ship it once those comments have been applied. Thanks for all the work you did on this!



================
Comment at: libcxx/docs/index.rst:121-125
+.. note::
+
+   .. [#note-modules] The C++23 ``std`` module has additional requirement.
+                      This is an optional feature, see :ref:`ModulesInLibcxx`.
+
----------------
I think I would drop this note. Modules are very experimental at the moment, and adding a ref to that from our main documentation seems unnecessary and perhaps even undesirable.


================
Comment at: libcxx/modules/CMakeLists.txt.in:41-43
+if(NOT @LIBCXX_ENABLE_FSTREAM@)
+  message(FATAL_ERROR "Modules without fstream support is not yet implemented.")
+endif()
----------------
This has been rolled up into `LIBCXX_ENABLE_FILESYSTEM` now, so `LIBCXX_ENABLE_FSTREAM` doesn't exist anymore.


================
Comment at: libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp:60-69
+    std::string_view s = *list;
+    auto b             = s.begin();
+    auto e             = std::find(b, s.end(), ' ');
+    while (b != e) {
+      decls_.emplace(b, e);
+      if (e == s.end())
+        break;
----------------
Mordante wrote:
> Mordante wrote:
> > ldionne wrote:
> > > We should figure out why `std::views::split(*list, ' ')` stopped working at some point and use that instead. You can loop in @var-const when you have a reproducer. Ditto below.
> > > 
> > > If this ends up needing a fix in `<ranges>`, I won't block the review on this comment of course but it would be nice to at least see if it's just a dumb mistake.
> > Odd taking the code from D144994 now works again :-/ I didn't investigate why. I much rather use that code, there is a reason why I used that code in the first place.
> I turns out that this code isn't compiled against libc++, but against the system C++ library. So that might explain the earlier failure I saw; I probably tested it on a system with an older GCC. (I noticed it when looking at the output of the most recent CI failure.)
The question at hand is really what version of standard libraries do we support in our build tools. We've never had that question because we have very little C++ in our build tools (only the clang tidy checks) -- everything else is pretty much Python. I think what we want to do here is enforce that our build tools (that means anything we use in order to build or test the library) are written using the following requirements:
1. C++17 (like the rest of non-libc++ LLVM)
2. The compilers supported by libc++ (which are stricter than those supported by LLVM)
3. The standard libraries supported by the rest of non-libc++ LLVM

Concretely, this means that we basically lower all of our restrictions to be the same as non-libc++ LLVM for our build tools. I think this makes sense and should have little impact on us.

We could document that under `tools/README.md` or in our other rst documentation around contributing. Let's make it clear that this is only for build tools, though. I would also do this on a best effort basis, i.e. I wouldn't go crazy and try to add a build job that checks our minimum requirement, that seems overkill (especially given the load on our CI).


================
Comment at: libcxx/utils/libcxx/test/dsl.py:440
 
+class AddModule(ConfigAction):
+  def applyTo(self, config):
----------------



================
Comment at: libcxx/utils/libcxx/test/dsl.py:462-463
+
+  def pretty(self, config, litParams):
+    pass
 
----------------



================
Comment at: libcxx/utils/libcxx/test/params.py:128
             ),  # AppleClang disregards -fmodules entirely when compiling C++. This enables modules for C++.
+            # TODO(LLVM-18) Remove the True backwards compatibility.
+        ]
----------------
I don't think this comment is relevant anymore.


================
Comment at: libcxx/utils/libcxx/test/params.py:141-146
+            AddSubstitution(
+                "%{link_flag}",
+                lambda cfg: os.path.join(
+                    cfg.test_exec_root, "__config_module__/libc++std.a"
+                ),
+            ),
----------------
I think what you meant here is `AddLinkFlag(os.path.join(cfg.test_exec_root, "__config_module__/libc++std.a"))`.

Also, it would be interesting to understand why your tests worked without linking against the std module as-is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151814