[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/Features.def:233
 FEATURE(modules, LangOpts.Modules)
+FEATURE(builtin_headers_in_system_modules, LangOpts.BuiltinHeadersInSystemModules)
 FEATURE(safe_stack, LangOpts.Sanitize.has(SanitizerKind::SafeStack))
----------------
benlangmuir wrote:
> This appears to be in a list of `// C++ TSes`.  Near the bottom there is 'Miscellaneous language extensions' which might be a better fit.
I thought it went with `FEATURE(modules, LangOpts.Modules)`, but I can put it down in 'Miscellaneous language extensions' if that's better.


================
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2851
+  if (!SDKInfo)
+    return false;
+  
----------------
benlangmuir wrote:
> I would have expected the default to be true; do old SDKs not have SDKInfo or something?
I'm not sure if we ever expect there to not be SDKInfo, but I thought it safer to default to the current behavior which is false.


================
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:2856
+  case Darwin::MacOS:
+    return SDKVersion >= VersionTuple(100U);
+  case Darwin::IPhoneOS:
----------------
benlangmuir wrote:
> Are these really supposed to be 100 or is this a placeholder?
Placeholder until we actually update the SDKs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159483