[libc++][modules] Adds the C++23 std module.

[Mordante]

Summary

The patch is based on D144994.

D151030 added the module definitions for the module std.
This patch wires in the module and enables the basic testing.

Some notable features are missing:

• There is no test that libc++ can be fully imported as a module.
• This lacks the parts for the std.compat module.
• The module is not shipped with libc++.

Implements parts of

• P2465R3 Standard Library Modules std and std.compat

[ldionne]

libcxx/docs/Modules.rst

59

74–76

95–96

110–111

libcxx/docs/ReleaseNotes.rst

114–120

I would move this to Build System Changes. Also, I think we can just change it, no need for a deprecation period here. We can consider our Lit test suite as something internal -- even though some vendors may have to make a few changes, those are easy to make and they don't affect end-users. Let's not go through more hoops than we need to.

139–141

This can go.

libcxx/test/libcxx/module_std.gen.py

67

92–94

96

129

This makes it a bit clearer what the file contains.

151

157

158

I think you can just test for if skip_declarations since an empty list/string is false-y in Python.

162

195

224–230

I don't think this is needed at all.

libcxx/test/libcxx/modules_include.sh.cpp

1 ↗

(On Diff #528202)

I think you need to rebase onto main.

libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp

24

61–70

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.

135

Per our discussion, I don't think this is an issue.

181–190

I think we can do

return name.starts_with("std::__") || (name.starts_with("std::_) && std::isupper(name[6]));

libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.hpp

12

I think we should IWYU properly here.

17

IDK what's the usual convention here, but do we want this to be explicit?

libcxx/utils/ci/buildkite-pipeline.yml

149

libcxx/utils/ci/run-buildbot

418–419

Can we do this in buildkite-pipeline.yml instead? Otherwise we can't run this job locally.

libcxx/utils/generate_header_tests.py

76 ↗

(On Diff #528202)

This will go away upon rebasing.

libcxx/utils/libcxx/test/dsl.py

446

448

libcxx/utils/libcxx/test/params.py

83–87

Not necessary.

132–136

I think we could simplify the CMake/Lit integration quite a bit by instead just building the BMIs from the top-level CMake invocation instead of from within Lit itself. Then we'd only need to pass the value for -fprebuilt-module-path to Lit and we wouldn't need AddModule(), the bridge changes, and also we might not need to use absolute paths everywhere.

I think this is worth investigating since it would simplify this patch quite a bit.

[Mordante]

Thanks for the reviews!

libcxx/test/libcxx/module_std.gen.py

59–61

It's indeed black's doing. Changing this to lists improves the formatting.

92–94

I just added it directly to the list, without an append. The comment can be in the list.

157

This doesn't work, it might try to add [] to a string.
Instead I assigned the joined list to skip_declarations.

libcxx/test/libcxx/modules_include.sh.cpp

1 ↗

(On Diff #528202)

I will do that shortly before our next review, then I can move the changes to the .cppm files in their own review.

libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp

61–70

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.

181–190

I tried that and it doesn't work. Three are elements in std::ranges too. I updated the comment.

libcxx/utils/ci/buildkite-pipeline.yml

149

As mention during review I kept std in the name since std.compat needs its own job.

libcxx/utils/libcxx/test/dsl.py

446–447

@aaronmondal actually I think I know what the issue is. CMake does not know CMAKE_CXX_STANDARD 26. The current development version of CMake has this support for Clang.

libcxx/utils/libcxx/test/params.py

132–136

I tried that and it doesn't work. The module needs to be build with the flags given to lit and the standard version used. Since this can change when lit is invoked we need to do that during the test.

What I did is building a module from CMake and then rebuilt it from the lit test. That way I need 2 instead of 4 new elements in the bridge. I feel the {modules} directory is not too bad to have, it matches {includes}. Only CMake is an oddity, I hope/expect that can be removed once CMake has proper module support.

[Mordante]

Addresses review comments.

[Mordante]

CI fixes.

[Mordante]

CI fixes.

[Mordante]

Adds some notes for reviewing.

libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp

61–70

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.)

62

The code is not used for Clang < 17 since these compilers lack module support. However there is a working implementation for these versions of Clang. The original code fails in https://buildkite.com/llvm-project/libcxx-ci/builds/25748. Since we'll drop LLVM 15 support rather soon, I prefer this work-around for the short term.

Looking at the build log, it seems to be an issue with concepts:

clang_tidy_checks/header_exportable_declarations.cpp:9:

In file included from /usr/lib/llvm-17/include/clang-tidy/ClangTidyCheck.h:12:

In file included from /usr/lib/llvm-17/include/clang-tidy/ClangTidyDiagnosticConsumer.h:12:

In file included from /usr/lib/llvm-17/include/clang-tidy/ClangTidyOptions.h:12:

In file included from /usr/lib/llvm-17/include/llvm/ADT/IntrusiveRefCntPtr.h:66:

In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/memory:66:

In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_tempbuf.h:61:

In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_construct.h:61:

In file included from /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_iterator_base_types.h:71:

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/iterator_concepts.h:999:13: error: no matching function for call to '__begin'

        = decltype(ranges::__cust_access::__begin(std::declval<_Tp&>()));

                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/ranges_base.h:513:5: note: in instantiation of template type alias '__range_iter_t' requested here

    using iterator_t = std::__detail::__range_iter_t<_Tp>;

    ^

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/ranges_util.h:131:36: note: in instantiation of template type alias 'iterator_t' requested here

      requires contiguous_iterator<iterator_t<_Derived>>

                                   ^

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:1134:29: note: in instantiation of template class 'std::ranges::view_interface<std::ranges::ref_view<llvm::StringRef>>' requested here

    class ref_view : public view_interface<ref_view<_Range>>

                            ^

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:1269:38: note: in instantiation of template class 'std::ranges::ref_view<llvm::StringRef>' requested here

        concept __can_ref_view = requires { ref_view{std::declval<_Range>()}; };

                                            ^

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:1269:38: note: in instantiation of requirement here

        concept __can_ref_view = requires { ref_view{std::declval<_Range>()}; };

                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:1269:27: note: (skipping 8 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)

        concept __can_ref_view = requires { ref_view{std::declval<_Range>()}; };

                                 ^

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:3717:6: note: while substituting template arguments into constraint expression here

          = requires { split_view(std::declval<_Range>(), std::declval<_Pattern>()); };

            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:3723:11: note: while checking the satisfaction of concept '__can_split_view<llvm::StringRef &, char>' requested here

        requires __detail::__can_split_view<_Range, _Pattern>

                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

/usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/ranges:3723:11: note: while substituting template arguments into constraint expression here

        requires __detail::__can_split_view<_Range, _Pattern>

                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[Mordante]

libcxx/modules/std/string.cppm

74

Except for this change all other changes in this directory should be in a separate commit. For now I keep them here. @H-G-Hristov is making great progress on P1614 (the spaceship paper). This means a lot of changes to the to the modules. After this patch is accepted I will coordinate with @H-G-Hristov regarding splitting this patch and avoid conflicts with the spaceship patches.

[Mordante]

Rebased.
Added CMake 3.27 suppprt, by adding the new experimental UUID.
Fixed the changes in the upstream headers. The ongoing spaceship patches change
a lot of modules.

[Mordante]

CI fixes

[ldionne]

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!

libcxx/docs/index.rst

122–126

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.

libcxx/modules/CMakeLists.txt.in

42–44

This has been rolled up into LIBCXX_ENABLE_FILESYSTEM now, so LIBCXX_ENABLE_FSTREAM doesn't exist anymore.

libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp

61–70

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).

libcxx/utils/libcxx/test/dsl.py

440

462–463

libcxx/utils/libcxx/test/params.py

128

I don't think this comment is relevant anymore.

141–146

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.

[Mordante]

In D151814#4417987, @ldionne wrote:

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!

Thanks a lot for your review!

libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp

61–70

I will do that in a followup patch.

libcxx/utils/libcxx/test/params.py

141–146

It seems to work without it. It might be something was/is needed for the std.compat module. For now I removed it.

[Mordante]

Rebased.
Removed more operator!=
Addresses review comments

[ldionne]

libcxx/test/libcxx/modules_include.gen.py

17

We should clean up the naming of these files in a separate patch to make it clear what's testing C++20 modules and what's testing Clang modules.

[Mordante]

libcxx/modules/std/string.cppm

74

I checked there are no open operator!= patches.

libcxx/test/libcxx/modules_include.gen.py

17

Good point. I'll make a followup patch for the renaming after landing this patch.

[Mordante]

libcxx/test/libcxx/modules_include.gen.py

17

I've created D153042

libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp

61–70

I've created D152377.

[ldionne]

libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp

61–70

I think that's referencing the wrong patch.