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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits