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