[PATCH] D159483: [Modules] Add a flag to control builtin headers being in system modules

Tue Sep 19 21:49:38 PDT 2023

iana marked 2 inline comments as done.
iana added inline comments.

================
Comment at: clang/include/clang/Basic/LangOptions.def:176
 COMPATIBLE_LANGOPT(CPlusPlusModules, 1, 0, "C++ modules syntax")
+LANGOPT(BuiltinHeadersInSystemModules, 1, 0, "builtin headers belong to system modules, and _Builtin_ modules are ignored for cstdlib headers")
 BENIGN_ENUM_LANGOPT(CompilingModule, CompilingModuleKind, 3, CMK_None,
----------------
vsapsai wrote:
> iana wrote:
> > vsapsai wrote:
> > > Why not to make the default value `true` to preserve the old behavior and switch on the new behavior in the driver? It's not a veiled way to force you to change it, I'm genuinely curious and want to consider pro/cons of various alternatives.
> > I did it this way because the current behavior is nonintuitive and just kind of bizarre. I had no idea that `Darwin.C.stdint` also included the compiler's stdint.h, and that the OS's stdint.h was treated textually. I thought it would be better for the default behavior to be for modules to behave consistently and obviously, not have secret special cases, and be usable with C++.
> > 
> > The con of course is that if anyone besides Apple depends on the current behavior they're going to either have to fix their OS modules or add this flag. I'm not sure if other platforms have a convenient driver class where they can do that.
> I'm fine with the more aggressive approach as long as we test Swift on Linux. That's the only project I know that is willing to keep using clang modules. For other projects if things break and it is a motivation to move to C++ modules, it is a positive direction, as for me.
> 
> Another reason I wanted to be more cautious is because of https://discourse.llvm.org/t/configure-script-breakage-with-the-new-werror-implicit-function-declaration/65213 change. Clang started enforcing 20+ years old rule and that has broken some stuff. So I don't see "you should've known better before relying on this behavior" as a good argument.
> 
> Anyway, for Swift testing here is what you can do (if you have your own plan, go ahead):
> 1. Make PR for the same change as this one at https://github.com/apple/llvm-project/
> 2. Create inconsequential PR at https://github.com/apple/swift/
> 3. Use [[ https://github.com/apple/swift/blob/main/docs/ContinuousIntegration.md#cross-repository-testing | Cross Repository Testing ]] to test clang change from swift on Linux.
If things break I think we would just add the flag to a few more drivers and fix it like that. I'm definitely not trying to drive anyone away from clang modules, I'm just hoping they don't need the strange current behavior. If it turns out to be too much of a mess then it can be on by default and we'll do an fno flag instead that we can turn off in the Darwin driver when we're ready.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159483