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

mib added a comment. @iana I believe this is affecting lldb macOS bot: https://green.lab.llvm.org/green/view/LLDB/job/as-lldb-cmake/6316/ Can you take a look ? Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159483/new/

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG0ea3d88bdb16: [Modules] Add a flag to control builtin headers being in system modules (authored by iana). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

dblaikie added subscribers: sammccall, dblaikie. dblaikie added a comment. @sammccall @rsmith - figure if this does impact us, we'll use the flag to opt-out in the short term while we figure out longer-term solution, yeah? Is there any pre-emptive testing you think is worth doing? Repository:

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

Bigcheese accepted this revision. Bigcheese added a comment. This revision is now accepted and ready to land. The implementation of this change looks good. I share the concern that changing the default may break other users, but that's easily remedied and this is the correct default. It would

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

iana marked 2 inline comments as done. iana added inline comments. 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

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

vsapsai added inline comments. 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

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

iana marked an inline comment as done. iana added inline comments. 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

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

vsapsai added a comment. In 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

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

iana updated this revision to Diff 556627. iana added a comment. __stddef_max_align_t.h always needs a header guard, because the type merger can't handle struct's. If it has a header guard then it always needs to have a module too, even if `-fbuiltin-headers-in-system-modules` is passed.

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

iana updated this revision to Diff 556279. iana added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159483/new/ https://reviews.llvm.org/D159483 Files: clang/include/clang/Basic/Features.def

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

iana updated this revision to Diff 556230. iana marked 3 inline comments as done. iana added a comment. Add comments to stdarg.h and stddef.h explaining their interesting header guard setup. Ignore -fbuiltin-headers-in-system-modules if -fmodules isn't passed. DriverKit doesn't need to pass

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

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,

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

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

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

iana updated this revision to Diff 556207. iana added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159483/new/ https://reviews.llvm.org/D159483 Files: clang/include/clang/Basic/Features.def

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

iana updated this revision to Diff 556206. iana added a comment. Fix formatting Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159483/new/ https://reviews.llvm.org/D159483 Files: clang/include/clang/Basic/Features.def

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

iana marked an inline comment as done. iana added a comment. In D159483#4641289 , @benlangmuir wrote: >>> How does this work today? Wouldn't the include guard prevent this? >> >> Today they don't define their include guard when building with modules. >

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

benlangmuir added a comment. >> How does this work today? Wouldn't the include guard prevent this? > > Today they don't define their include guard when building with modules. Thanks - I can see some headers behave that way, or in other cases there are other sorts of has_feature(modules) checks

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

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,

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

iana added a comment. In D159483#4641231 , @benlangmuir wrote: >> but need to be repeatedly included when they're used in system modules > > How does this work today? Wouldn't the include guard prevent this? Today they don't define their include guard

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

iana added inline comments. 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, I don't love this

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

benlangmuir added a comment. > but need to be repeatedly included when they're used in system modules How does this work today? Wouldn't the include guard prevent this? Comment at: clang/include/clang/Basic/Features.def:233 FEATURE(modules, LangOpts.Modules)

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

iana added a comment. 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

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

iana created this revision. iana added reviewers: aaron.ballman, rsmith, efriedma, ldionne, ChuanqiXu, Bigcheese, vsapsai, benlangmuir. Herald added a subscriber: ributzka. Herald added a project: All. iana requested review of this revision. Herald added subscribers: cfe-commits, MaskRay. Herald