[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:
> iana wrote:
> > 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.
> I think misc is probably better.
👍 


================
Comment at: clang/include/clang/Driver/Options.td:2944
           [NoXarchOption], [ClangOption, CLOption]>>;
+def fbuiltin_headers_in_system_modules : Flag <["-"], "fbuiltin-headers-in-system-modules">,
+  Group<f_Group>,
----------------
benlangmuir wrote:
> iana wrote:
> > I don't love this flag name, but I couldn't come up with anything more succinct. It actually does two things.
> > 
> >   # Turns on `ModuleMap::isBuiltinHeader` behavior, that is the builtin headers get added into the system modules.
> >   # Causes the module map parser to ignore all of the new builtin modules added in D159064. This is needed before the modules are added to assist with Apple internal staging with the Swift compiler.
> > 
> I think the name is good enough for a -cc1 option, unless someone else has a better suggestion. I suggest adding a HelpText - you could basically copy the description you added to LangOptions.def
Sure


================
Comment at: clang/lib/Lex/ModuleMap.cpp:1540
+    /// or added to Map's Modules/ModuleScopeIDs).
+    bool IgnoreModules = false;
+
----------------
benlangmuir wrote:
> iana wrote:
> > This an everything under it is gross, but the only other thing I could think of was to duplicate the module map and decided to load a special one for BuiltinHeadersInSystemModules. That seemed weirder.
> Would it be possible to get the right semantics using a `requires` decl in the modules plus a new module feature?
Maybe? I guess e.g. _Builtin_stdint would be seen, the new requires flag would be seen, and if it's not satisfied then its headers wouldn't be added to the module? That seems like a really weird use of requires though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159483