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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
