This only shows up if one passes module maps to Clang in -std=c++20
modes, e.g. for layering checks.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
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?
| clang/lib/Driver/ToolChains/Clang.cpp | ||
|---|---|---|
| 3631 | 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? 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) | |
| 6542 | this change looks correct but unrelated, can we split into a separate change? | |
Comment Actions
- Revert gnu++20 fix
- Always pass -fheader-modules/-fno-header-modules when modules are enabled
Comment Actions
@rsmith could you confirm you are happy with the changes (and naming)?
And let us know if there are others we should loop in.
| clang/lib/Driver/ToolChains/Clang.cpp | ||
|---|---|---|
| 3631 | Makes sense, I think it's clearer. | |
| 6542 | Good point, will send it as a separate review. | |
Comment Actions
Header modules are part of the C++20 standard (where they are called "header units"), and module maps are an intended way for Clang to provide this functionality in C++20 mode. I don't think turning this off by default in C++20 is the right forward-looking plan; rather, I think we should be thinking about moving towards header modules simply always being something that Clang can do, with no flag to control that.
That said: this new flag is not actually turning off header modules -- we'll still treat headers listed in module maps as having modular semantics with this flag specified (especially if -fmodules-local-submodule-visibility is enabled, but some modules semantics such as checking for undeclared dependencies are applied regardless). Rather, this flag will cause us to parse header modules on the fly, as part of the same parsing action, rather than building and importing a separate AST for them. And I think that is a reasonable thing to have a flag for. Maybe -fheader-modules=parse (to parse them on the fly) versus -fheader-modules=import (to import them from a pcm file)? This would also leave the door open for being able to specify whether modular semantics are enabled for header modules or not, as a different value for the same flag.
Comment Actions
It feels unclear that using Clang's legacy module maps functionality _is_ a reasonable forward-looking way to provide this functionality for the C++20 standard. It feels to me like the models are fairly distinct and we might be better off disentangling C++20 modules from module-maps entirely (e.g. that we may continue to support Clang modules in C++20 for compatibility, but not as a way to support any C++20 standard features).
Has there been some previous discussion on this topic that concluded that we do want module maps for C++20 modules support?
Comment Actions
Richard, thanks for course correcting. I was under impression that header modules are not in the standard, my mistake.
It looks like this particular change actually breaks standard compatibility as we also use the same parsing action and don't build a module separately on import.
[module.import]p5 A module-import-declaration that specifies a header-name H imports a synthesized header unit, which is a translation unit formed by applying phases 1 to 7 of translation (5.2) to the source file or header nominated by H, which shall not contain a module-declaration.
We are definitely allowed to use the same parsing action for #include .
[cpp.include]p7 If the header identified by the header-name denotes an importable header (10.3), it is implementation-defined whether the #include preprocessing directive is instead replaced by an import directive (15.5) of the form
So the proposal is that -fheader-modules=parse would parse #include of header unit in the same TU, and import .pcm on import, right?
To take a step back, the original problem was to allow running layering checks without requiring .pcm files even in -std=c++20.
Are there alternatives I might be missing?
Comment Actions
So the proposal is that -fheader-modules=parse would parse #include of header unit in the same TU, and import .pcm on import, right?
Per cpp.includep7, "If the header identified by the header-name denotes an importable header ([module.import]), it is implementation-defined whether the #include preprocessing directive is instead replaced by an import directive ([cpp.import]) of the form
import header-name ; new-line"
For Clang, I would expect the implementation-defined behavior to derive from the existence of a module map that nominates a header as being importable.
Comment Actions
Sure, that's what Clang does now. It's totally reasonable.
It seems we can get away with our use-case by passing -fno-modules flag as we don't really use C++20 modules. It should still provide the layering check.
Comment Actions
For the record, the intention is that (with local submodule visibility enabled) we behave "as if" we preprocessed separately, even if we do so using the same parsing action: we store various parts of the preprocessor state (most notably, the defined macros map) per-module, so that when we switch modules, we get the same preprocessing behavior as if we'd started from a clean slate. Fundamentally, Clang's model is that a single ASTContext and a single preprocessing action can cover multiple translation units, and that includes the possibility that a single preprocessor or Sema object can process multiple translation units, and we make this work by tracking which macros and declarations and similar should be visible where, based on which modules are currently imported. This is intended to be a conforming strategy for implementing C++20 header units.
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)