[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

bruno closed this revision. bruno added a comment. Thanks for the review Richard. Committed 291644 Comment at: lib/Lex/HeaderSearch.cpp:1082 + auto TryEnterImported = [&](void) -> bool { +if (!ModulesEnabled) rsmith wrote: > Maybe add a FIXME here

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: lib/Lex/HeaderSearch.cpp:1082 + auto TryEnterImported = [&](void) -> bool { +if (!ModulesEnabled) Maybe add a FIXME here indicating

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

bruno added a comment. @rsmith ping! https://reviews.llvm.org/D26267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

bruno added a comment. Ping! https://reviews.llvm.org/D26267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

bruno updated this revision to Diff 82067. bruno marked 3 inline comments as done. bruno added a comment. Uploaded new patch with another approach to fix the issue. https://reviews.llvm.org/D26267 Files: include/clang/Lex/HeaderSearch.h include/clang/Lex/ModuleMap.h

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

bruno marked 3 inline comments as done. bruno added a comment. > Do we need to perfectly preserve the functionality of `#pragma once` / > `#import`? That is, would it be acceptable to > you if we spuriously enter files that have been imported in that way, while > building a module?

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

rsmith added a comment. Other options: Do we need to perfectly preserve the functionality of `#pragma once` / `#import`? That is, would it be acceptable to you if we spuriously enter files that have been imported in that way, while building a module? If we view them as pure optimization, we

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

bruno updated this revision to Diff 81478. bruno added a comment. Here is the Decl dump in the end of `ReadDeclRecord` for each Decl before it fails: - ParmVarDecl 0x7ffe23053b20 col:12 in libc.math hidden 'int' -- FunctionDecl 0x7ffe23053a48 col:5 in libc.math hidden abs 'int (int)'

Re: [PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

On 9 December 2016 at 03:59, Richard Smith via Phabricator via cfe-commits < cfe-commits@lists.llvm.org> wrote: > rsmith added a comment. > > Looks like we might only be making macros visible, and failing to emit the > annot_module_import token for Sema. Hmm, no, we inject that token

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

rsmith added a comment. Looks like we might only be making macros visible, and failing to emit the annot_module_import token for Sema. https://reviews.llvm.org/D26267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

bruno added a comment. Any thoughts on this @rsmith ? https://reviews.llvm.org/D26267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

bruno added a comment. > The right thing to do would be to track the set of headers whose #import / > #include-with-#pragma-once is > visible, and only skip inclusions if there is a *visible* #import / > #include-with-#pragma-once of that header. This makes sense, but the situation is a bit

Re: [PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

On 30 November 2016 at 11:08, Bruno Cardoso Lopes via Phabricator via cfe-commits wrote: > > I suspect the problem is instead that #import just doesn't work properly > with modules > > (specifically, either with local submodule visibility or with modules > that do not

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

bruno added inline comments. Comment at: lib/Frontend/FrontendActions.cpp:227 +IsBuiltin = true; + addHeaderInclude(H.NameAsWritten, Includes, LangOpts, Module->IsExternC, + IsBuiltin /*AlwaysInclude*/); rsmith wrote: > I don't

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

rsmith added inline comments. Comment at: lib/Frontend/FrontendActions.cpp:227 +IsBuiltin = true; + addHeaderInclude(H.NameAsWritten, Includes, LangOpts, Module->IsExternC, + IsBuiltin /*AlwaysInclude*/); I don't understand why

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

bruno added a comment. @rsmith ping! https://reviews.llvm.org/D26267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

bruno added a comment. Ping! https://reviews.llvm.org/D26267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

bruno updated this revision to Diff 76931. bruno marked an inline comment as done. bruno added a comment. Update patch after Vassil's comments! https://reviews.llvm.org/D26267 Files: include/clang/Lex/ModuleMap.h lib/Frontend/FrontendActions.cpp lib/Lex/ModuleMap.cpp

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

bruno marked an inline comment as done. bruno added a comment. In https://reviews.llvm.org/D26267#586971, @v.g.vassilev wrote: > Could you include more context when creating the diff eg. git diff -U, or > equivalent. I did, -U9 actually, not sure why you're not getting it...

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

v.g.vassilev added a comment. Could you include more context when creating the diff eg. git diff -U, or equivalent. Comment at: include/clang/Lex/ModuleMap.h:278 + /// headers. + static bool isBuiltinHeader(StringRef FileName); + It seems this is in the

[PATCH] D26267: [Modules] Include builtins with #include instead of #import for ObjC

bruno created this revision. bruno added a reviewer: rsmith. bruno added subscribers: cfe-commits, manmanren. After r284797 builins are treated like textual includes. When compiling for ObjC++, the in-memory header file generated for the modules is composed only of #import's instead of