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++] Simplify how we select modules flavors in the test suite #66385

Merged
merged 1 commit into from
Sep 18, 2023

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Sep 14, 2023

This gets rid of the separate parameter enable_modules_lsv in favor of adding a named option to the enable_modules parameter. The patch also removes the getModuleFlag helper, which was just a really complicated way of hardcoding "none".

This gets rid of the separate parameter enable_modules_lsv in favor of
adding a named option to the enable_modules parameter. The patch also
removes the getModuleFlag helper, which was just a really complicated
way of hardcoding "none".
@ldionne ldionne requested a review from mordante September 14, 2023 14:51
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 14, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 14, 2023

@llvm/pr-subscribers-libcxx

Changes This gets rid of the separate parameter enable_modules_lsv in favor of adding a named option to the enable_modules parameter. The patch also removes the getModuleFlag helper, which was just a really complicated way of hardcoding "none". -- Full diff: https://github.com//pull/66385.diff

2 Files Affected:

  • (modified) libcxx/cmake/caches/Generic-modules-lsv.cmake (+1-1)
  • (modified) libcxx/utils/libcxx/test/params.py (+15-31)
diff --git a/libcxx/cmake/caches/Generic-modules-lsv.cmake b/libcxx/cmake/caches/Generic-modules-lsv.cmake
index 66b76134c282b8a..395fccc21765066 100644
--- a/libcxx/cmake/caches/Generic-modules-lsv.cmake
+++ b/libcxx/cmake/caches/Generic-modules-lsv.cmake
@@ -1,2 +1,2 @@
-set(LIBCXX_TEST_PARAMS "enable_modules=clang;enable_modules_lsv=True" CACHE STRING "")
+set(LIBCXX_TEST_PARAMS "enable_modules=clang-lsv" CACHE STRING "")
 set(LIBCXXABI_TEST_PARAMS "${LIBCXX_TEST_PARAMS}" CACHE STRING "")
diff --git a/libcxx/utils/libcxx/test/params.py b/libcxx/utils/libcxx/test/params.py
index e05c6689964c734..6fe466ec0c6f595 100644
--- a/libcxx/utils/libcxx/test/params.py
+++ b/libcxx/utils/libcxx/test/params.py
@@ -87,15 +87,6 @@ def getStdFlag(cfg, std):
     return None
 
 
-_allModules = ["none", "clang"]
-
-
-def getModuleFlag(cfg, enable_modules):
-    if enable_modules in _allModules:
-        return enable_modules
-    return None
-
-
 # fmt: off
 DEFAULT_PARAMETERS = [
     Parameter(
@@ -128,32 +119,25 @@ def getModuleFlag(cfg, enable_modules):
     ),
     Parameter(
         name="enable_modules",
-        choices=_allModules,
+        choices=["none", "clang", "clang-lsv"],
         type=str,
-        help="Whether to build the test suite with modules enabled. Select "
-        "`clang` for Clang modules",
-        default=lambda cfg: next(s for s in _allModules if getModuleFlag(cfg, s)),
-        actions=lambda enable_modules: [
-            AddFeature("clang-modules-build"),
-            AddCompileFlag("-fmodules"),
-            AddCompileFlag("-fcxx-modules"), # AppleClang disregards -fmodules entirely when compiling C++. This enables modules for C++.
+        help="Whether to build the test suite with modules enabled. "
+             "Select `clang` for Clang modules, and 'clang-lsv' for Clang modules with Local Submodule Visibility.",
+        default="none",
+        actions=lambda modules: filter(None, [
+            AddFeature("clang-modules-build")           if modules in ("clang", "clang-lsv") else None,
+
+            # Note: AppleClang disregards -fmodules entirely when compiling C++, so we also pass -fcxx-modules
+            #       to enable modules for C++.
+            AddCompileFlag("-fmodules -fcxx-modules")   if modules in ("clang", "clang-lsv") else None,
+
             # Note: We use a custom modules cache path to make sure that we don't reuse
             #       the default one, which can be shared across CI builds with different
             #       configurations.
-            AddCompileFlag(lambda cfg: f"-fmodules-cache-path={cfg.test_exec_root}/ModuleCache"),
-        ]
-        if enable_modules == "clang"
-        else [],
-    ),
-    Parameter(
-        name="enable_modules_lsv",
-        choices=[True, False],
-        type=bool,
-        default=False,
-        help="Whether to enable Local Submodule Visibility in the Modules build.",
-        actions=lambda lsv: [] if not lsv else [
-            AddCompileFlag("-Xclang -fmodules-local-submodule-visibility"),
-        ],
+            AddCompileFlag(lambda cfg: f"-fmodules-cache-path={cfg.test_exec_root}/ModuleCache") if modules in ("clang", "clang-lsv") else None,
+
+            AddCompileFlag("-Xclang -fmodules-local-submodule-visibility") if modules == "clang-lsv" else None,
+        ])
     ),
     Parameter(
         name="enable_exceptions",

@ldionne ldionne requested a review from a team September 15, 2023 15:37
@ldionne ldionne merged commit d4d8f21 into llvm:main Sep 18, 2023
@ldionne ldionne deleted the review/simplify-modules-lit branch September 18, 2023 13:37
ZijunZhaoCCK pushed a commit to ZijunZhaoCCK/llvm-project that referenced this pull request Sep 19, 2023
…vm#66385)

This gets rid of the separate parameter enable_modules_lsv in favor of
adding a named option to the enable_modules parameter. The patch also
removes the getModuleFlag helper, which was just a really complicated
way of hardcoding "none".
zahiraam pushed a commit to tahonermann/llvm-project that referenced this pull request Oct 24, 2023
…vm#66385)

This gets rid of the separate parameter enable_modules_lsv in favor of
adding a named option to the enable_modules parameter. The patch also
removes the getModuleFlag helper, which was just a really complicated
way of hardcoding "none".
zahiraam pushed a commit to tahonermann/llvm-project that referenced this pull request Oct 24, 2023
…vm#66385)

This gets rid of the separate parameter enable_modules_lsv in favor of
adding a named option to the enable_modules parameter. The patch also
removes the getModuleFlag helper, which was just a really complicated
way of hardcoding "none".
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.

3 participants