[libcxx-commits] [PATCH] D157364: [libc++][Modules] Generate the __std_clang_module header

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Thanks for working on this! In general happy with the changes, but I like to see the CI green before approving.



================
Comment at: libcxx/test/libcxx/assertions/headers_declare_verbose_abort.gen.py:17
 sys.path.append(sys.argv[1])
-from libcxx.test.header_information import lit_header_restrictions, public_headers
+from libcxx.header_information import lit_header_restrictions, public_headers
 
----------------
I would like to see this change and its related changes in a separate review.


================
Comment at: libcxx/utils/CMakeLists.txt:7
+  COMMAND "${Python3_EXECUTABLE}" "${CMAKE_CURRENT_SOURCE_DIR}/generate_std_clang_module_header.py"
+  COMMENT "Generate the <__std_clang_module> header")
+
----------------



================
Comment at: libcxx/utils/generate_std_clang_module_header.py:1
+import operator
+import os.path
----------------
Please add a copyright block.


================
Comment at: libcxx/utils/generate_std_clang_module_header.py:28
+//===----------------------------------------------------------------------===//
+
+// This header should not be directly included, it's exclusively to import all
----------------
Similar warnings are used in other generated headers.


================
Comment at: libcxx/utils/generate_std_clang_module_header.py:47
+    )
+    for header in sorted(always_available_headers):
+        std_clang_module_header.write("#include <")
----------------
The format script complains about the sorting, mainly hunks like
```
-#include <limits>
 #include <limits.h>
+#include <limits>
```
I suspect this is something what happens on Apple where the ordering of `.` and '>` differs. I know that @ldionne has seen this too. Would it help to force the C locale? This is something we use in other places where we sort data in the CI.


================
Comment at: libcxx/utils/generate_std_clang_module_header.py:48-50
+        std_clang_module_header.write("#include <")
+        std_clang_module_header.write(header)
+        std_clang_module_header.write(">\n")
----------------
I feel this is a bit more readable, can you look at using f-strings at other places too.


================
Comment at: libcxx/utils/libcxx/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",
----------------
I would like to see these two changes committed separately. When the CI is green feel free to commit these two.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157364