Address review comments

mordante · mordante · commit a95db48a745c · 2024-01-09T20:07:01.000+01:00
diff --git a/libcxx/test/libcxx/module_std.gen.py b/libcxx/test/libcxx/module_std.gen.py
@@ -29,7 +29,7 @@
     "%{clang-tidy}",
     "%{test-tools}/clang_tidy_checks/libcxx-tidy.plugin",
     "%{cxx}",
-    "%{flags} %{compile_flags} %{module_flags}",
+    "%{flags} %{compile_flags}",
     "std",
 )
 
diff --git a/libcxx/test/libcxx/module_std_compat.gen.py b/libcxx/test/libcxx/module_std_compat.gen.py
@@ -29,7 +29,7 @@
     "%{clang-tidy}",
     "%{test-tools}/clang_tidy_checks/libcxx-tidy.plugin",
     "%{cxx}",
-    "%{flags} %{compile_flags} %{module_flags}",
+    "%{flags} %{compile_flags}",
     "std.compat",
 )
 
diff --git a/libcxx/test/libcxx/selftest/modules/no-modules.sh.cpp b/libcxx/test/libcxx/selftest/modules/no-modules.sh.cpp
@@ -6,7 +6,10 @@
 //
 //===----------------------------------------------------------------------===//
 
-// Make sure that the module flags are empty when no module is supplied.
+// Make sure that the compile flags contain no module information.
 
-// MODULES:
-// RUN: echo "%{module_flags}"  | grep "^$"
+// MODULE_DEPENDENCIES:
+
+// RUN: echo "%{compile_flags}" | grep -v -- "-fprebuilt-module-path="
+// RUN: echo "%{compile_flags}" | grep -v "std.pcm"
+// RUN: echo "%{compile_flags}" | grep -v "std.compat.pcm"
diff --git a/libcxx/test/libcxx/selftest/modules/std-and-std.compat-module.sh.cpp b/libcxx/test/libcxx/selftest/modules/std-and-std.compat-module.sh.cpp
@@ -10,14 +10,14 @@
 // UNSUPPORTED: clang-modules-build
 // UNSUPPORTED: gcc
 
-// XFAIL: has-no-module-support
+// XFAIL: has-no-cxx-module-support
 
-// Make sure that the module flags contain the expected elements.
+// Make sure that the compile flags contain the expected elements.
 // The tests only look for the expected components and not the exact flags.
 // Otherwise changing the location of the module breaks this test.
 
-// MODULES: std std.compat
-//
-// RUN: echo "%{module_flags}" | grep -- "-fprebuilt-module-path="
-// RUN: echo "%{module_flags}" | grep "std.pcm"
-// RUN: echo "%{module_flags}" | grep "std.compat.pcm"
+// MODULE_DEPENDENCIES: std std.compat
+
+// RUN: echo "%{compile_flags}" | grep -- "-fprebuilt-module-path="
+// RUN: echo "%{compile_flags}" | grep "std.pcm"
+// RUN: echo "%{compile_flags}" | grep "std.compat.pcm"
diff --git a/libcxx/test/libcxx/selftest/modules/std-module.sh.cpp b/libcxx/test/libcxx/selftest/modules/std-module.sh.cpp
@@ -10,16 +10,16 @@
 // UNSUPPORTED: clang-modules-build
 // UNSUPPORTED: gcc
 
-// XFAIL: has-no-module-support
+// XFAIL: has-no-cxx-module-support
 
-// Make sure that the module flags contain the expected elements.
+// Make sure that the compile flags contain the expected elements.
 // The tests only look for the expected components and not the exact flags.
 // Otherwise changing the location of the module breaks this test.
 
-// MODULES: std
-//
-// RUN: echo "%{module_flags}" | grep -- "-fprebuilt-module-path="
-// RUN: echo "%{module_flags}" | grep "std.pcm"
+// MODULE_DEPENDENCIES: std
+
+// RUN: echo "%{compile_flags}" | grep -- "-fprebuilt-module-path="
+// RUN: echo "%{compile_flags}" | grep "std.pcm"
 
 // The std module should not provide the std.compat module
-// RUN: echo "%{module_flags}" | grep -v "std.compat.pcm"
+// RUN: echo "%{compile_flags}" | grep -v "std.compat.pcm"
diff --git a/libcxx/test/libcxx/selftest/modules/std.compat-module.sh.cpp b/libcxx/test/libcxx/selftest/modules/std.compat-module.sh.cpp
@@ -10,16 +10,16 @@
 // UNSUPPORTED: clang-modules-build
 // UNSUPPORTED: gcc
 
-// XFAIL: has-no-module-support
+// XFAIL: has-no-cxx-module-support
 
-// Make sure that the module flags contain the expected elements.
+// Make sure that the compile flags contain the expected elements.
 // The tests only look for the expected components and not the exact flags.
 // Otherwise changing the location of the module breaks this test.
 
-// MODULES: std.compat
-//
-// RUN: echo "%{module_flags}" | grep -- "-fprebuilt-module-path="
-// RUN: echo "%{module_flags}" | grep "std.compat.pcm"
+// MODULE_DEPENDENCIES: std.compat
+
+// RUN: echo "%{compile_flags}" | grep -- "-fprebuilt-module-path="
+// RUN: echo "%{compile_flags}" | grep "std.compat.pcm"
 
 // It's unspecified whether std.compat is built on the std module.
 // Therefore don't test its presence
diff --git a/libcxx/test/libcxx/selftest/modules/unknown-module.compile.pass.cpp b/libcxx/test/libcxx/selftest/modules/unknown-module.compile.pass.cpp
diff --git a/libcxx/test/std/modules/std.compat.pass.cpp b/libcxx/test/std/modules/std.compat.pass.cpp
@@ -10,11 +10,11 @@
 // UNSUPPORTED: clang-modules-build
 // UNSUPPORTED: gcc
 
-// XFAIL: has-no-module-support
+// XFAIL: has-no-cxx-module-support
 
 // A minimal test to validate import works.
 
-// MODULES: std.compat
+// MODULE_DEPENDENCIES: std.compat
 
 import std.compat;
 
diff --git a/libcxx/test/std/modules/std.pass.cpp b/libcxx/test/std/modules/std.pass.cpp
@@ -10,11 +10,11 @@
 // UNSUPPORTED: clang-modules-build
 // UNSUPPORTED: gcc
 
-// XFAIL: has-no-module-support
+// XFAIL: has-no-cxx-module-support
 
 // A minimal test to validate import works.
 
-// MODULES: std
+// MODULE_DEPENDENCIES: std
 
 import std;
 
diff --git a/libcxx/utils/libcxx/test/features.py b/libcxx/utils/libcxx/test/features.py
@@ -319,8 +319,8 @@ def _getAndroidDeviceApi(cfg):
     ),
     # Whether module support for the platform is available.
     Feature(
-        name="has-no-module-support",
-        # The libc of these platforms have functions with internal linkages.
+        name="has-no-cxx-module-support",
+        # The libc of these platforms have functions with internal linkage.
         # This is not allowed per C11 7.1.2 Standard headers/6
         #  Any declaration of a library function shall have external linkage.
         when=lambda cfg: "__ANDROID__" in compilerMacros(cfg)
diff --git a/libcxx/utils/libcxx/test/format.py b/libcxx/utils/libcxx/test/format.py
@@ -52,6 +52,21 @@ def _executeScriptInternal(test, litConfig, commands):
     return (out, err, exitCode, timeoutInfo, parsedCommands)
 
 
+def _validateModuleDependencies(modules):
+    for m in modules:
+        if m not in ("std", "std.compat"):
+            raise RuntimeError(
+                f"Invalid module dependency '{m}', only 'std' and 'std.compat' are valid"
+            )
+
+
+def _getSubstitution(substitution, config):
+    for (orig, replacement) in config.substitutions:
+        if orig == substitution:
+            return replacement
+    raise ValueError("Substitution {} is not in the config.".format(substitution))
+
+
 def parseScript(test, preamble):
     """
     Extract the script from a test, with substitutions applied.
@@ -105,7 +120,7 @@ def parseScript(test, preamble):
             initial_value=additionalCompileFlags,
         ),
         lit.TestRunner.IntegratedTestKeywordParser(
-            "MODULES:",
+            "MODULE_DEPENDENCIES:",
             lit.TestRunner.ParserKind.SPACE_LIST,
             initial_value=modules,
         ),
@@ -138,65 +153,62 @@ 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
-        elif module == "std.compat":
-            has_std_compat_module = True
-        else:
-            script.insert(
-                0,
-                f"echo \"The module '{module}' is not valid, use 'std' or 'std.compat'\"",
-            )
-            script.insert(1, "false")
-            return script
+    # Add compile flags specified with ADDITIONAL_COMPILE_FLAGS.
+    # Modules need to be build with the same compilation flags as the
+    # test. So add these flags before adding the modules.
+    substitutions = [
+        (s, x + " " + " ".join(additionalCompileFlags))
+        if s == "%{compile_flags}"
+        else (s, x)
+        for (s, x) in substitutions
+    ]
 
     if modules:
+        _validateModuleDependencies(modules)
+
         # This flag is needed for both modules.
         moduleCompileFlags.append("-fprebuilt-module-path=%T")
 
-        # Building the modules needs to happen before the other script commands
-        # are executed. Therefore the commands are added to the front of the
-        # list.
-        if has_std_compat_module:
+        # The moduleCompileFlags are added to the %{compile_flags}, but
+        # the modules need should be built without these flags. So
+        # expand the compile_flags and add the expanded value to the
+        # build script.
+        compileFlags = _getSubstitution("%{compile_flags}", test.config)
+
+        # Building the modules needs to happen before the other script
+        # commands are executed. Therefore the commands are added to the
+        # front of the list.
+        if "std.compat" in modules:
             script.insert(
                 0,
-                "%dbg(MODULE std.compat) %{cxx} %{flags} %{compile_flags} "
+                "%dbg(MODULE std.compat) %{cxx} %{flags} "
+                f"{compileFlags} "
                 "-Wno-reserved-module-identifier -Wno-reserved-user-defined-literal "
                 "--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.
-        # 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")
-
-    # Add compile flags specified with ADDITIONAL_COMPILE_FLAGS.
-    substitutions = [
-        (s, x + " " + " ".join(additionalCompileFlags))
-        if s == "%{compile_flags}"
-        else (s, x)
-        for (s, x) in substitutions
-    ]
-    # In order to use modules, additional compilation flags are required.
-    # Adding these to the %{compile_flags} gives a chicken and egg issue:
-    # - the modules need to be built with the same compilation flags as the
-    #   tests,
-    # - except for the module dependency, which does not exist.
-    # The issue is resolved by adding a private substitution.
-    substitutions.append(("%{module_flags}", " ".join(moduleCompileFlags)))
+        # Make sure the std module is added before std.compat. Libc++'s
+        # std.compat module will depend 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.
+        script.insert(
+            0,
+            "%dbg(MODULE std) %{cxx} %{flags} "
+            f"{compileFlags} "
+            "-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 required for the modules.
+        substitutions = [
+            (s, x + " " + " ".join(moduleCompileFlags))
+            if s == "%{compile_flags}"
+            else (s, x)
+            for (s, x) in substitutions
+        ]
 
     # Perform substitutions in the script itself.
     script = lit.TestRunner.applySubstitutions(
@@ -250,7 +262,6 @@ class CxxStandardLibraryTest(lit.formats.FileBasedTest):
     constructs:
         %{cxx}           - A command that can be used to invoke the compiler
         %{compile_flags} - Flags to use when compiling a test case
-        %{module_flags}  - Flags to use when compiling a test case that imports modules
         %{link_flags}    - Flags to use when linking a test case
         %{flags}         - Flags to use either when compiling or linking a test case
         %{exec}          - A command to prefix the execution of executables
@@ -287,7 +298,7 @@ class CxxStandardLibraryTest(lit.formats.FileBasedTest):
 
            This directive will build the required C++23 standard library
            modules and add the additional compiler flags in
-           %{module_flags}. (Libc++ offers these modules in C++20 as an
+           %{compile_flags}. (Libc++ offers these modules in C++20 as an
            extension.)
 
     Additional provided substitutions and features
@@ -354,22 +365,22 @@ def execute(self, test, litConfig):
             ".compile.pass.mm"
         ):
             steps = [
-                "%dbg(COMPILED WITH) %{cxx} %s %{flags} %{compile_flags} %{module_flags} -fsyntax-only"
+                "%dbg(COMPILED WITH) %{cxx} %s %{flags} %{compile_flags} -fsyntax-only"
             ]
             return self._executeShTest(test, litConfig, steps)
         elif filename.endswith(".compile.fail.cpp"):
             steps = [
-                "%dbg(COMPILED WITH) ! %{cxx} %s %{flags} %{compile_flags} %{module_flags} -fsyntax-only"
+                "%dbg(COMPILED WITH) ! %{cxx} %s %{flags} %{compile_flags} -fsyntax-only"
             ]
             return self._executeShTest(test, litConfig, steps)
         elif filename.endswith(".link.pass.cpp") or filename.endswith(".link.pass.mm"):
             steps = [
-                "%dbg(COMPILED WITH) %{cxx} %s %{flags} %{compile_flags} %{module_flags} %{link_flags} -o %t.exe"
+                "%dbg(COMPILED WITH) %{cxx} %s %{flags} %{compile_flags} %{link_flags} -o %t.exe"
             ]
             return self._executeShTest(test, litConfig, steps)
         elif filename.endswith(".link.fail.cpp"):
             steps = [
-                "%dbg(COMPILED WITH) %{cxx} %s %{flags} %{compile_flags} %{module_flags} -c -o %t.o",
+                "%dbg(COMPILED WITH) %{cxx} %s %{flags} %{compile_flags} -c -o %t.o",
                 "%dbg(LINKED WITH) ! %{cxx} %t.o %{flags} %{link_flags} -o %t.exe",
             ]
             return self._executeShTest(test, litConfig, steps)
@@ -387,7 +398,7 @@ def execute(self, test, litConfig):
         # suffixes above too.
         elif filename.endswith(".pass.cpp") or filename.endswith(".pass.mm"):
             steps = [
-                "%dbg(COMPILED WITH) %{cxx} %s %{flags} %{compile_flags} %{module_flags} %{link_flags} -o %t.exe",
+                "%dbg(COMPILED WITH) %{cxx} %s %{flags} %{compile_flags} %{link_flags} -o %t.exe",
                 "%dbg(EXECUTED AS) %{exec} %t.exe",
             ]
             return self._executeShTest(test, litConfig, steps)
diff --git a/libcxx/utils/libcxx/test/modules.py b/libcxx/utils/libcxx/test/modules.py
@@ -126,7 +126,7 @@ def write_lit_configuration(self):
 // The GCC compiler flags are not always compatible with clang-tidy.
 // UNSUPPORTED: gcc
 
-// MODULES: {self.module}
+// MODULE_DEPENDENCIES: {self.module}
 
 // RUN: echo -n > {self.tmp_prefix}.all_partitions
 """

Original file line number	Diff line number	Diff line change
`@@ -29,7 +29,7 @@`
`29`	`29`	`"%{clang-tidy}",`
`30`	`30`	`"%{test-tools}/clang_tidy_checks/libcxx-tidy.plugin",`
`31`	`31`	`"%{cxx}",`
`32`		`- "%{flags} %{compile_flags} %{module_flags}",`
	`32`	`+ "%{flags} %{compile_flags}",`
`33`	`33`	`"std",`
`34`	`34`	`)`
`35`	`35`