[libcxx-commits] [PATCH] D156907: [libc++][modules] Removes the module partitions.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

This LGTM with some minor comments.

I am not overly concerned with maintainability, it doesn't seem a lot worse than what we had before. Concretely, I think that the amount of interaction with these files by libc++ developers is going to be quite limited, and IMO working on improving how we report failure-to-have-updated the `.inc` files is what will yield the most value for our workflow.

I would tend not to backport this. First, we're not shipping C++20 modules to users, so backporting would have no effect. Also, this is experimental and it's technically not a bug (although that's arguable), so I feel like we wouldn't be following our usual policy for backports if we did.



================
Comment at: libcxx/modules/README.md:4
+The files in this directory contain the exported named declarations per header.
+These files are used for the following purposes
+
----------------



================
Comment at: libcxx/modules/README.md:13
+at different locations. This means the user of these "partitions" are
+responsible for including the proper header and validate whether the header can
+be loaded in the current libc++ configuration. For example "include <locale>"
----------------



================
Comment at: libcxx/modules/README.md:19-23
+Originally the files in this directory were real C++ module partitions. It
+turned out module partitions caused overhead when using the std module. This
+currently used method is a lot faster. The drawback is the files in this
+directory are no longer selfcontained. The performance issue was discussed on
+[Discourse](https://discourse.llvm.org/t/alternatives-to-the-implementation-of-std-modules/71958).
----------------



================
Comment at: libcxx/test/libcxx/module_std.gen.py:146-152
+    # Generate a module partition for the header module includes.
+    # Original these files were real module partition. This had a negative
+    # impact on the compilation speed. The advantage of these files was that it
+    # was easy to track which headers didn't export all their named declarations.
+    # This helper makes it possible again to test the exports per header.
+    #
+    # TODO MODULES This needs to take the header restrictions into account.
----------------
IMO the historical part of the comment is useful in this review, but not in the code.


================
Comment at: libcxx/test/libcxx/module_std.gen.py:158-160
+        f"// new is a keyword so use __libcpp_module_new instead.{nl}"
+        f"// Just using __new fails for thread and complex.{nl}"
+        f"export module std:__libcpp_module_{header};{nl}"
----------------
That seems to explain why we're doing this a bit more explicitly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156907