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

[Ian Anderson via Phabricator via cfe-commits]

iana marked an inline comment 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:
> 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.


================
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2860-2861
+    return SDKVersion >= VersionTuple(99U);
+  default:
+    return true;
+  }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159483