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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a reviewer: rsmith.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.

In D125773#3519048 <https://reviews.llvm.org/D125773#3519048>, @sammccall wrote:

> 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?

@rsmith could you confirm you are happy with the changes (and naming)?
And let us know if there are others we should loop in.



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3631
+  // Unless '-fmodules' was specified explicitly.
+  if (!HaveClangModules && HaveModules) {
+    CmdArgs.push_back("-fno-header-modules");
----------------
sammccall wrote:
> 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)
Makes sense, I think it's clearer.
As for using `false` as a default, the mentioned cleanup is that we need to update all tests to specify `-fheader-modules` explicitly.
I'm also not sure if there are any guarantees for `-cc1 ` that we have to adhere to. My assumption is that `-cc1` has no stability guarantees, but I'm not 100% certain.


================
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[] = {
----------------
sammccall wrote:
> 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)
Good point, will send it as a separate review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125773