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

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai 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,
----------------
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.


================
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2860-2861
+    return SDKVersion >= VersionTuple(99U);
+  default:
+    return true;
+  }
----------------
iana wrote:
> vsapsai wrote:
> > Another question regarding defaults. Doesn't look consistent with your position
> > > [...] I thought it safer to default to the current behavior which is false.
> > 
> > Personally I gravitate towards `false` for extra safety but it's not a strong opinion.
> default only covers DriverKit, which can be true. Any future platforms should also default to true.
Ok, that looks like a reasonable choice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159483