[libcxx-commits] [PATCH] D156177: [libc++][Modules] Recreate the top level `std` clang module

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this! In general LGTM!
The 2 comments in `libcxx/include/__std_clang_module` can be done in the generator patch and are informative.
The 2 other comments should be addressed before landing this patch.

After you land this patch can you create a backport request and ping me on Discord. Then I can approve the request and hopefully get this in LLVM-17 before RC-1.

@aprantl @JDevlieghere thanks for your patience!



================
Comment at: libcxx/docs/Contributing.rst:50
 
+  - Did you add it to ``include/__std``?
   - Did you add it to ``include/module.modulemap.in``?
----------------
iana wrote:
> Mordante wrote:
> > Since we're going to add a script, so this can be removed.
> Discussed in Discord, I'll make a script in a separate review later today, I'll remove this comment then.
I would like to remove it anyway. We want to backport this patch to LLVM-17 which then contains incorrect information. For main the information will be wrong for a few days; I don't expect new headers in that short time-span.


================
Comment at: libcxx/include/__std_clang_module:154
+
+#ifdef _LIBCPP_AVAILABILITY_TO_CHARS_FLOATING_POINT
+#  include <print>
----------------
Similar as below this header can always be included. For now keep it, just keep it in mind for the generator script.


================
Comment at: libcxx/include/__std_clang_module:185
+
+#if _LIBCPP_STD_VER >= 14
+#  ifndef _LIBCPP_HAS_NO_THREADS
----------------
Typically we don't use the language version guards. They also seem to be incomplete. For example `<print>` is C++23 only.
I don't mind it for this patch, it's more a remark to keep in mind for the generator patch.

Including `<print>` in C++03 does compile and not give diagnostics.
Including `<locale>` with `_LIBCPP_HAS_NO_LOCALIZATION` will trigger an `#error "foo"` in the pre-processor.
That's not clear from the annotations in the python file since that is used for the test where we indeed need to disable tests in older language versions.


================
Comment at: libcxx/utils/libcxx/test/header_information.py:18-21
-    "experimental/algorithm": "// UNSUPPORTED: c++03",
     "experimental/deque": "// UNSUPPORTED: c++03",
     "experimental/forward_list": "// UNSUPPORTED: c++03",
-    "experimental/functional": "// UNSUPPORTED: c++03",
----------------
When the CI is green can you commit this separately. I like to not have this in this patch. Normally I don't mind some additional polishing, but I want to keep the changes for LLVM-17 as small as possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156177