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

[Ian Anderson via Phabricator via libcxx-commits]

iana added inline comments.


================
Comment at: libcxx/include/CMakeLists.txt:982-986
+  if (LIBCXX_GENERATED_INCLUDE_DIR STREQUAL LIBCXX_GENERATED_INCLUDE_TARGET_DIR)
+    set(libcxx_include_target_directory "")
+  else()
+    set(libcxx_include_target_directory --libcxx-include-target-directory ${LIBCXX_GENERATED_INCLUDE_TARGET_DIR})
+  endif()
----------------
Is there a nicer way to conditionally include command options? I'm not super pro with CMake.


================
Comment at: libcxx/include/generate_module_map.py:53
+def write_libcxx_includes(libcxx_include_directory: str, libcxx_includes_file: typing.TextIO) -> None:
+    for directory, subdirectorynames, filenames in os.walk(libcxx_include_directory):
+        if directory == libcxx_include_directory:
----------------
Arguably `libcxx_include_target_directory` headers should also be modularized since they ultimately get merged into the libc++ include directory. But doing so would require gymnastics and probably an llvm/clang vfs to make it work locally and for tests. It only affects `__config_site` at the moment, which can continue to skate by as nonmodular for now. And it happens to not be a problem on Apple platforms anyway since they don't use a distinct target include directory.


================
Comment at: libcxx/include/generate_module_map.py:56
+            include_root = ''
+            # The ext, __locale_dir, __support headers are not included in the module.
+            # Not all of the __pstl headers compile, so exclude that directory too, even
----------------
They probably should be included in the module map, but they aren't in the current module map and supporting them would be tricky since some of the headers don't compile on some platforms.


================
Comment at: libcxx/include/generate_module_map.py:57-58
+            # The ext, __locale_dir, __support headers are not included in the module.
+            # Not all of the __pstl headers compile, so exclude that directory too, even
+            # though some of the headers will be pulled in transitively and modularized.
+            subdirectorynames.remove('ext')
----------------
This should probably be fixed too, but pstl looks like it's still changing, so that part can wait I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144322