[libcxx-commits] [PATCH] D144322: [libc++][Modules] Make top level modules for all C++ headers with OS/clang versions

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/docs/Contributing.rst:44
+  - If it's a public header, did you add it to ``include/module.modulemap.in``?
+  - If it's a private header, does it need special clang module attributes in include/private_header_module_attributes.json.in?
 
----------------
Slight bikeshedding but I prefer a different name. Private sounds to me it doesn't offer public visible declarations, like the headers in src.


================
Comment at: libcxx/include/__algorithm/pstl_any_all_none_of.h:12
 
+#include <__algorithm/move.h>
+#include <__algorithm/pstl_backend.h>
----------------
IIRC these have already been merged in separate patches, right?


================
Comment at: libcxx/include/__threading_support:43
+// Include <math.h> here to work around that.
+# include <math.h>
 # include <pthread.h>
----------------
Has this been part of a separate patch?


================
Comment at: libcxx/include/generate_module_map.py:7
+#
+#===----------------------------------------------------------------------===##
+
----------------
@ldionne maybe we should considering this script to generate our transitive includes in the future.


================
Comment at: libcxx/include/generate_module_map.py:30
+#    the includees of the other module. In other words, if they're at the same
+#    level of the module tree.
+def main() -> None:
----------------
Nice documentation!


================
Comment at: libcxx/include/generate_module_map.py:46
+        header_module_attributes = json.load(module_attributes_file)
+    include_tree = build_include_tree(args.cxx_compiler, args.target, args.isysroot, args.libcxx_include_directory, args.libcxx_include_target_directory, libcxx_includes_path, header_module_attributes)
+    private_headers_by_directory = collect_transitive_includes(include_tree)
----------------
This line is very long. It would be great to use black as discussed here
https://discourse.llvm.org/t/rfc-document-and-standardize-python-code-style/68257/20

Note that line 79 really requires a large screen to fit on one line. I will review this file later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144322