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

Mon Sep 18 18:12:59 PDT 2023

vsapsai added a comment.

In D159483#4641191 <https://reviews.llvm.org/D159483#4641191>, @iana wrote:

> A big assumption this patch makes is that `ModuleMap::isBuiltinHeader` is primarily to support Apple's unfortunate module needs. Thus this patch turns that behavior off by default, which makes things work the way one would expect. That is, when usr/include/module.modulemap references stdint.h, that just means usr/include/stdint.h and it doesn't also pull in the clang builtin stdint.h, it doesn't transform usr/include/stdint.h into a textual header, etc. I'm hoping that's acceptable behavior on non-Apple platforms, but if someone knows otherwise please let me know and we can rethink how the option should be defined and set.

Good way to test the assumptions can be checking how Swift works with this change on Linux. There are not as many module maps there as in Apple SDKs but there are some.

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

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159483