Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++][modules] Improves std.compat module. #76330

Merged
merged 2 commits into from
Jan 21, 2024

Conversation

mordante
Copy link
Member

Let the std.compat module use the std module instead of duplicating the exports.

Based on @ChuanqiXu9's suggestion in #71438.

@mordante mordante requested a review from a team as a code owner December 24, 2023 11:15
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Dec 24, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 24, 2023

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

Let the std.compat module use the std module instead of duplicating the exports.

Based on @ChuanqiXu9's suggestion in #71438.


Full diff: https://github.com/llvm/llvm-project/pull/76330.diff

7 Files Affected:

  • (modified) libcxx/modules/std.compat.cppm.in (+2-123)
  • (modified) libcxx/modules/std.cppm.in (+1)
  • (modified) libcxx/test/libcxx/module_std_compat.gen.py (+2-23)
  • (modified) libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp (+1-1)
  • (modified) libcxx/utils/generate_libcxx_cppm_in.py (+4-2)
  • (modified) libcxx/utils/libcxx/header_information.py (+25)
  • (modified) libcxx/utils/libcxx/test/format.py (+10-11)
diff --git a/libcxx/modules/std.compat.cppm.in b/libcxx/modules/std.compat.cppm.in
index f199e194e60b16..651d6ec7b9fe26 100644
--- a/libcxx/modules/std.compat.cppm.in
+++ b/libcxx/modules/std.compat.cppm.in
@@ -17,38 +17,17 @@ module;
 
 // The headers of Table 24: C++ library headers [tab:headers.cpp]
 // and the headers of Table 25: C++ headers for C library facilities [tab:headers.cpp.c]
-#include <algorithm>
-#include <any>
-#include <array>
-#if !defined(_LIBCPP_HAS_NO_ATOMIC_HEADER)
-#  include <atomic>
-#endif
-#if !defined(_LIBCPP_HAS_NO_THREADS)
-#  include <barrier>
-#endif
-#include <bit>
-#include <bitset>
 #include <cassert>
 #include <cctype>
 #include <cerrno>
 #include <cfenv>
 #include <cfloat>
-#include <charconv>
-#include <chrono>
 #include <cinttypes>
 #include <climits>
 #if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
 #  include <clocale>
 #endif
 #include <cmath>
-#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-#  include <codecvt>
-#endif
-#include <compare>
-#include <complex>
-#include <concepts>
-#include <condition_variable>
-#include <coroutine>
 #include <csetjmp>
 #include <csignal>
 #include <cstdarg>
@@ -65,107 +44,6 @@ module;
 #if !defined(_LIBCPP_HAS_NO_WIDE_CHARACTERS)
 #  include <cwctype>
 #endif
-#include <deque>
-#include <exception>
-#include <execution>
-#include <expected>
-#include <filesystem>
-#include <format>
-#include <forward_list>
-#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-#  include <fstream>
-#endif
-#include <functional>
-#if !defined(_LIBCPP_HAS_NO_THREADS)
-#  include <future>
-#endif
-#include <initializer_list>
-#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-#  include <iomanip>
-#endif
-#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-#  include <ios>
-#endif
-#include <iosfwd>
-#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-#  include <iostream>
-#endif
-#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-#  include <istream>
-#endif
-#include <iterator>
-#if !defined(_LIBCPP_HAS_NO_THREADS)
-#  include <latch>
-#endif
-#include <limits>
-#include <list>
-#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-#  include <locale>
-#endif
-#include <map>
-#include <mdspan>
-#include <memory>
-#include <memory_resource>
-#include <mutex>
-#include <new>
-#include <numbers>
-#include <numeric>
-#include <optional>
-#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-#  include <ostream>
-#endif
-#include <print>
-#include <queue>
-#include <random>
-#include <ranges>
-#include <ratio>
-#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-#  include <regex>
-#endif
-#include <scoped_allocator>
-#if !defined(_LIBCPP_HAS_NO_THREADS)
-#  include <semaphore>
-#endif
-#include <set>
-#if !defined(_LIBCPP_HAS_NO_THREADS)
-#  include <shared_mutex>
-#endif
-#include <source_location>
-#include <span>
-#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-#  include <sstream>
-#endif
-#include <stack>
-#include <stdexcept>
-#if !defined(_LIBCPP_HAS_NO_THREADS)
-#  include <stop_token>
-#endif
-#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-#  include <streambuf>
-#endif
-#include <string>
-#include <string_view>
-#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-#  include <strstream>
-#endif
-#if !defined(_LIBCPP_HAS_NO_LOCALIZATION)
-#  include <syncstream>
-#endif
-#include <system_error>
-#if !defined(_LIBCPP_HAS_NO_THREADS)
-#  include <thread>
-#endif
-#include <tuple>
-#include <type_traits>
-#include <typeindex>
-#include <typeinfo>
-#include <unordered_map>
-#include <unordered_set>
-#include <utility>
-#include <valarray>
-#include <variant>
-#include <vector>
-#include <version>
 
 // *** Headers not yet available ***
 #if __has_include(<debugging>)
@@ -203,6 +81,7 @@ module;
 #endif // __has_include(<text_encoding>)
 
 export module std.compat;
+export import std;
+
 
-@LIBCXX_MODULE_STD_INCLUDE_SOURCES@
 @LIBCXX_MODULE_STD_COMPAT_INCLUDE_SOURCES@
\ No newline at end of file
diff --git a/libcxx/modules/std.cppm.in b/libcxx/modules/std.cppm.in
index b46c52e781f82f..6ce8e287737b88 100644
--- a/libcxx/modules/std.cppm.in
+++ b/libcxx/modules/std.cppm.in
@@ -204,4 +204,5 @@ module;
 
 export module std;
 
+
 @LIBCXX_MODULE_STD_INCLUDE_SOURCES@
diff --git a/libcxx/test/libcxx/module_std_compat.gen.py b/libcxx/test/libcxx/module_std_compat.gen.py
index 63fdd8188937e1..033ec34513c572 100644
--- a/libcxx/test/libcxx/module_std_compat.gen.py
+++ b/libcxx/test/libcxx/module_std_compat.gen.py
@@ -21,6 +21,7 @@
 import sys
 
 sys.path.append(sys.argv[1])
+from libcxx.header_information import module_c_headers
 from libcxx.test.modules import module_test_generator
 
 generator = module_test_generator(
@@ -37,27 +38,5 @@
 print("//--- module_std_compat.sh.cpp")
 generator.write_test(
     "std.compat",
-    [
-        "cassert",
-        "cctype",
-        "cerrno",
-        "cfenv",
-        "cfloat",
-        "cinttypes",
-        "climits",
-        "clocale",
-        "cmath",
-        "csetjmp",
-        "csignal",
-        "cstdarg",
-        "cstddef",
-        "cstdint",
-        "cstdio",
-        "cstdlib",
-        "cstring",
-        "ctime",
-        "cuchar",
-        "cwchar",
-        "cwctype",
-    ],
+    module_c_headers,
 )
diff --git a/libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp b/libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp
index 15d2e6839ad5e3..d2ab96321cb80a 100644
--- a/libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp
+++ b/libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp
@@ -95,7 +95,7 @@ header_exportable_declarations::header_exportable_declarations(
 
   list = Options.get("ExtraDeclarations");
   // TODO(LLVM-17) Remove clang 15 work-around.
-#if defined(__clang_major__) && __clang_major__ < 16
+#if defined(__clang_major__) && __clang_major__ < 19
   if (list) {
     std::string_view s = *list;
     auto b             = s.begin();
diff --git a/libcxx/utils/generate_libcxx_cppm_in.py b/libcxx/utils/generate_libcxx_cppm_in.py
index f957406778d392..2d3f829847fb9f 100644
--- a/libcxx/utils/generate_libcxx_cppm_in.py
+++ b/libcxx/utils/generate_libcxx_cppm_in.py
@@ -9,6 +9,7 @@
 import os.path
 import sys
 
+from libcxx.header_information import module_c_headers
 from libcxx.header_information import module_headers
 from libcxx.header_information import header_restrictions
 from libcxx.header_information import headers_not_available
@@ -44,7 +45,7 @@ def write_file(module):
 // and the headers of Table 25: C++ headers for C library facilities [tab:headers.cpp.c]
 """
         )
-        for header in module_headers:
+        for header in module_headers if module == "std" else module_c_headers:
             if header in header_restrictions:
                 module_cpp_in.write(
                     f"""\
@@ -69,8 +70,9 @@ def write_file(module):
         module_cpp_in.write(
             f"""
 export module {module};
+{'export import std;' if module == 'std.compat' else ''}
 
-@LIBCXX_MODULE_STD_INCLUDE_SOURCES@
+{'@LIBCXX_MODULE_STD_INCLUDE_SOURCES@' if module == 'std' else ''}
 {'@LIBCXX_MODULE_STD_COMPAT_INCLUDE_SOURCES@' if module == 'std.compat' else ''}"""
         )
 
diff --git a/libcxx/utils/libcxx/header_information.py b/libcxx/utils/libcxx/header_information.py
index 54e18b5ea533dd..528c05e63564b8 100644
--- a/libcxx/utils/libcxx/header_information.py
+++ b/libcxx/utils/libcxx/header_information.py
@@ -209,3 +209,28 @@ def is_modulemap_header(header):
     # These headers have been removed in C++20 so are never part of a module.
     and not header in ["ccomplex", "ciso646", "cstdbool", "ctgmath"]
 ]
+
+# The C headers used in the std and std.compat modules.
+module_c_headers = [
+    "cassert",
+    "cctype",
+    "cerrno",
+    "cfenv",
+    "cfloat",
+    "cinttypes",
+    "climits",
+    "clocale",
+    "cmath",
+    "csetjmp",
+    "csignal",
+    "cstdarg",
+    "cstddef",
+    "cstdint",
+    "cstdio",
+    "cstdlib",
+    "cstring",
+    "ctime",
+    "cuchar",
+    "cwchar",
+    "cwctype",
+]
diff --git a/libcxx/utils/libcxx/test/format.py b/libcxx/utils/libcxx/test/format.py
index 2840f7c29a3b10..e5cdf5688f8929 100644
--- a/libcxx/utils/libcxx/test/format.py
+++ b/libcxx/utils/libcxx/test/format.py
@@ -138,11 +138,10 @@ def parseScript(test, preamble):
     script += preamble
     script += scriptInTest
 
-    has_std_module = False
     has_std_compat_module = False
     for module in modules:
         if module == "std":
-            has_std_module = True
+            pass
         elif module == "std.compat":
             has_std_compat_module = True
         else:
@@ -165,23 +164,23 @@ def parseScript(test, preamble):
                 0,
                 "%dbg(MODULE std.compat) %{cxx} %{flags} %{compile_flags} "
                 "-Wno-reserved-module-identifier -Wno-reserved-user-defined-literal "
+                "-fprebuilt-module-path=%T %T/std.pcm "
                 "--precompile -o %T/std.compat.pcm -c %{module}/std.compat.cppm",
             )
             moduleCompileFlags.append("%T/std.compat.pcm")
 
         # Make sure the std module is added before std.compat.
-        # Libc++'s std.compat module will depend on its std module.
+        # Libc++'s std.compat module depends on its std module.
         # It is not known whether the compiler expects the modules in the order
         # of their dependencies. However it's trivial to provide them in that
         # order.
-        if has_std_module:
-            script.insert(
-                0,
-                "%dbg(MODULE std) %{cxx} %{flags} %{compile_flags} "
-                "-Wno-reserved-module-identifier -Wno-reserved-user-defined-literal "
-                "--precompile -o %T/std.pcm -c %{module}/std.cppm",
-            )
-            moduleCompileFlags.append("%T/std.pcm")
+        script.insert(
+            0,
+            "%dbg(MODULE std) %{cxx} %{flags} %{compile_flags} "
+            "-Wno-reserved-module-identifier -Wno-reserved-user-defined-literal "
+            "--precompile -o %T/std.pcm -c %{module}/std.cppm",
+        )
+        moduleCompileFlags.append("%T/std.pcm")
 
     # Add compile flags specified with ADDITIONAL_COMPILE_FLAGS.
     substitutions = [

# include <barrier>
#endif
#include <bit>
#include <bitset>
#include <cassert>
#include <cctype>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try to remove the duplicated headers as much as possible. Maybe we can improve it by something like:

export module std.compat;
export import std;

export using double_t = std::double_t;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to make this change in this PR and investigate it separately. I want to make sure it doesn't make it harder for libc++ to test our modules. I created a new issue for it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough.

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks OK to me, but I'd like to see it once the parent patches have landed.

Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I don't actually need to see this again if the CI passes once this is rebased on top of the parent changes.

@mordante mordante force-pushed the users/mordante/fixes_clang-tidy_exports branch from dd147d3 to 3d82f80 Compare January 17, 2024 07:26
Base automatically changed from users/mordante/fixes_clang-tidy_exports to main January 17, 2024 07:27
@mordante mordante force-pushed the users/mordante/improves_std.compat_module branch 2 times, most recently from 6adf12f to ede2a4c Compare January 17, 2024 12:04
Copy link

github-actions bot commented Jan 17, 2024

⚠️ Python code formatter, darker found issues in your code. ⚠️

You can test this locally with the following command:
darker --check --diff -r b689e1678c429b724dd898edb9e24cbb9c437667...191a14793fb04095edac9744aa25d9367e62cdc3 libcxx/test/libcxx/module_std_compat.gen.py libcxx/utils/generate_libcxx_cppm_in.py libcxx/utils/libcxx/header_information.py libcxx/utils/libcxx/test/format.py
View the diff from darker here.
--- utils/libcxx/test/format.py	2024-01-18 17:49:47.000000 +0000
+++ utils/libcxx/test/format.py	2024-01-18 17:52:29.139271 +0000
@@ -169,11 +169,11 @@
             script.insert(
                 0,
                 "%dbg(MODULE std.compat) %{cxx} %{flags} "
                 f"{compileFlags} "
                 "-Wno-reserved-module-identifier -Wno-reserved-user-defined-literal "
-                "-fmodule-file=std=%T/std.pcm " # The std.compat module imports std.
+                "-fmodule-file=std=%T/std.pcm "  # The std.compat module imports std.
                 "--precompile -o %T/std.compat.pcm -c %{module}/std.compat.cppm",
             )
             moduleCompileFlags.extend(
                 ["-fmodule-file=std.compat=%T/std.compat.pcm", "%T/std.compat.pcm"]
             )

Let the std.compat module use the std module instead of duplicating the
exports.

Based on @ChuanqiXu9's suggestion in #71438.
@mordante mordante force-pushed the users/mordante/improves_std.compat_module branch from 05eea78 to 191a147 Compare January 18, 2024 17:49
@mordante mordante merged commit 0475733 into main Jan 21, 2024
44 of 45 checks passed
@mordante mordante deleted the users/mordante/improves_std.compat_module branch January 21, 2024 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants