[PATCH] D125773: [Driver] Do not auto-enable header modules with -std=c++20

[Sam McCall via Phabricator via cfe-commits]

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

LG, but I think this is choosing a (new) public name for clang modules/header modules, so maybe Richard or others might want to weigh in?



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3631
+  // Unless '-fmodules' was specified explicitly.
+  if (!HaveClangModules && HaveModules) {
+    CmdArgs.push_back("-fno-header-modules");
----------------
These interacting flags are complicated, and I think relying on how the default value is computed makes it harder to reason about.

Can we explicitly set -f{no}-header-modules whenever any version of modules is available?
i.e.
```
if (HaveModules) {
  if (HaveClangModules)
    push("-fheader-modules")
  else
    push ("-fno-header-modules")
}
```

(In the long run, it would be much clearer for -fheader-modules to default to false, and explicitly set it whenever it's needed, but this is a larger cleanup)


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6537
   // support by default, or just assume that all languages do.
-  bool HaveModules =
-      Std && (Std->containsValue("c++2a") || Std->containsValue("c++20") ||
-              Std->containsValue("c++latest"));
+  bool HaveModules = Std && llvm::any_of(Std->getValues(), [](const char *S) {
+                       constexpr llvm::StringRef CPP_MODULES_STD[] = {
----------------
this change looks correct but unrelated, can we split into a separate change?
(Mostly because either change might break stuff downstream, and they could be reverted separately)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125773