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

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

I suggest we add a comment explaining the weird has_feature checks being added here (even if they're less weird than what they're replacing).  Maybe just a comment in stdarg.h and/or stddef.h and then the __std(arg|def) headers could just add a one-liner reference  "see comment in std(arg|def).h".



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


================
Comment at: clang/include/clang/Driver/Options.td:2945-2946
+def fbuiltin_headers_in_system_modules : Flag <["-"], "fbuiltin-headers-in-system-modules">,
+  Group<f_Group>,
+  Flags<[NoXarchOption]>,
+  Visibility<[CC1Option]>,
----------------
iana wrote:
> I don't fully understand what these two do, but I think they apply here?
I agree they apply. `f_Group` affects a `ClaimAllArgs` call (via inheritance from `CompileOnly_Group`) but mostly puts it under a group in documentation.  `NoXarchOption` means you can't modify this option per-arch in a multi-arch compilation.  That seems like what you want.


================
Comment at: clang/lib/Lex/ModuleMap.cpp:1540
+    /// or added to Map's Modules/ModuleScopeIDs).
+    bool IgnoreModules = false;
+
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159483