[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

This revision was automatically updated to reflect the committed changes. Closed by commit rGf8536fb11e3d: [clang][HeaderSearch] Fix implicit module when using header maps (authored by ivanmurashko). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

benlangmuir accepted this revision. benlangmuir added a comment. This revision is now accepted and ready to land. > @benlangmuir, do you need any assistance from my side with it? @ivanmurashko thanks for your patience. I discussed this with some colleagues and we're in favour of making this

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

ivanmurashko updated this revision to Diff 525557. ivanmurashko added a comment. rebase to latest LLVM main branch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930 Files: clang/lib/Lex/HeaderSearch.cpp

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

ivanmurashko added a comment. > Still looking at issues and not sure whether these are blockers or not. Friendly reminder: @benlangmuir, do you need any assistance from my side with it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

ivanmurashko updated this revision to Diff 521580. ivanmurashko added a comment. typo fixed + rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930 Files: clang/lib/Lex/HeaderSearch.cpp

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

iana added a comment. I wonder if clang should have better module interaction with the header maps produced by Xcode, or if Xcode should produce better header maps to work with clang. Or are you having problems with header maps outside of Xcode? Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

iana added inline comments. Comment at: clang/test/Modules/implicit-module-header-maps.cpp:52 +//header and trip a `#error`, or +// 2) header maps aren't usesd, as the header name doesn't exist and relies on +//the header map to remap it to the real header.

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

benlangmuir added a comment. We have several new build failures with this change that I'm looking through. So far, a common one is an error of the form /source/module.modulemap: error: redefinition of module /build/Foo.framework/Modules/module.modulemap: note: previously defined here ie.

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

ivanmurashko updated this revision to Diff 520117. ivanmurashko added a comment. windows marked as non supported Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930 Files: clang/lib/Lex/HeaderSearch.cpp

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

ivanmurashko updated this revision to Diff 520096. ivanmurashko added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930 Files: clang/lib/Lex/HeaderSearch.cpp

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

ivanmurashko updated this revision to Diff 520069. ivanmurashko added a comment. small fix at the lit-test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930 Files: clang/lib/Lex/HeaderSearch.cpp

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

ivanmurashko updated this revision to Diff 520062. ivanmurashko added a comment. `split-file` was used for lit-test simplification (see @ChuanqiXu comment) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

benlangmuir added a comment. In D103930#4310061 , @ivanmurashko wrote: > Friendly ping > > @arphaman, @jansvoboda11, I have made the patch buildable on all platforms > and have all tests passed. There was also a small fix (temp path for modules >

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

ChuanqiXu added inline comments. Comment at: clang/test/Modules/Inputs/implicit-module-header-maps/a.h:1-3 +#ifdef FOO +#error foo +#endif It would be better to use `split-file` to merge the segments of the tests. You can find the example in the Modules

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

bruno added a comment. Thanks Duncan. > Given that header maps are somewhat Apple-specific Some non-obvious background: It began Apple specific, but Meta uses them at scale as well, pretty important for us to get this right. > and unit test coverage is a bit lacking for these sorts of

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

bruno added reviewers: Bigcheese, ChuanqiXu, jansvoboda11. bruno added a comment. Adding code owners and more relevant folks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

dexonsmith added subscribers: iana, ributzka. dexonsmith added a comment. In D103930#4310061 , @ivanmurashko wrote: > Friendly ping > > @arphaman, @jansvoboda11, I have made the patch buildable on all platforms > and have all tests passed. There was

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

ivanmurashko added a comment. Friendly ping @arphaman, @jansvoboda11, I have made the patch buildable on all platforms and have all tests passed. There was also a small fix (temp path for modules artefact) at the test that could fix its run on some platforms. Could you look at it? Does it

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

ivanmurashko updated this revision to Diff 517323. ivanmurashko added a comment. Windows constrains added Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930 Files: clang/lib/Lex/HeaderSearch.cpp

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

ivanmurashko updated this revision to Diff 517222. ivanmurashko added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930 Files: clang/lib/Lex/HeaderSearch.cpp

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

ivanmurashko updated this revision to Diff 517159. ivanmurashko added a comment. Commandeer the diff from @andrewjcg and made some chnages at the code (get it compatible with latest clang source code) and at the tests (move modules artefacts to temp folder to make the test execution more

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

andrewjcg added a comment. @ivanmurashko: Sorry for the delay getting back to you here. Feel free to commandeer, as I don't have plans to get to this soon. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

bruno added a comment. > Looks like this patch causes a number of issues for us, I will work with > @jansvoboda11 to see if there's some way to resolve them. If you can share a reproducer I'm pretty sure @ivanmurashko can help make it work for y'all too. Thanks! Repository: rG LLVM

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

ivanmurashko added a comment. @arphaman, @andrewjcg, what's the status of the diff? The functionality is important for me and I want to get it landed. BTW: @andrewjcg I can commander the diff if you don't mind. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

arphaman added a comment. Looks like this patch causes a number of issues for us, I will work with @jansvoboda11 to see if there's some way to resolve them. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

andrewjcg added a comment. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

arphaman added a comment. I've started testing this change. I'll let you know how it looks in a few days. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930 ___

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

jansvoboda11 added a comment. In D103930#2825181 , @dexonsmith wrote: > In D103930#2820725 , @bruno wrote: > >> Thanks for working on this, comments inline. @vsapsai @jansvoboda11 >> @dexonsmith any headermap

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

andrewjcg updated this revision to Diff 352898. andrewjcg added a comment. capitalize param Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930 Files: clang/lib/Lex/HeaderSearch.cpp

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

bruno added a comment. @dexonsmith and @jansvoboda11 thanks for the fast reply and the extra testing. Comment at: clang/lib/Lex/HeaderSearch.cpp:421 + auto FixupSearchPathAndFindUsableModule = + [&](auto file) -> Optional { if (SearchPath) { Can you

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

dexonsmith added inline comments. Comment at: clang/test/Modules/implicit-module-header-maps.cpp:27 +#define FOO +// This include will fail if modules weren't used. The include name itself +// doesn't exist and relies on the header map to remap it to the real header.

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

andrewjcg updated this revision to Diff 352824. andrewjcg added a comment. fix comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930 Files: clang/lib/Lex/HeaderSearch.cpp

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

andrewjcg added inline comments. Comment at: clang/test/Modules/implicit-module-header-maps.cpp:27 +#define FOO +// This include will fail if modules weren't used. The include name itself +// doesn't exist and relies on the header map to remap it to the real header.

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

andrewjcg added inline comments. Comment at: clang/test/Modules/implicit-module-header-maps.cpp:27 +#define FOO +// This include will fail if modules weren't used. The include name itself +// doesn't exist and relies on the header map to remap it to the real header.

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

dexonsmith added a subscriber: arphaman. dexonsmith added a comment. In D103930#2820725 , @bruno wrote: > Thanks for working on this, comments inline. @vsapsai @jansvoboda11 > @dexonsmith any headermap related concerns on your side? @jansvoboda11, I

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

andrewjcg updated this revision to Diff 352740. andrewjcg added a comment. fix sed for windows test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930 Files: clang/lib/Lex/HeaderSearch.cpp

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

jansvoboda11 added a comment. I don't have any comments regarding header maps. Comment at: clang/test/Modules/implicit-module-header-maps.cpp:17 +// +// RUN: sed -e "s:OUTPUTS_DIR:%t:g" %S/Inputs/implicit-module-header-maps/b.hmap.json > %t/hmap.json +// RUN: %hmaptool write

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

andrewjcg updated this revision to Diff 352299. andrewjcg added a comment. feedback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930 Files: clang/lib/Lex/HeaderSearch.cpp

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

bruno added subscribers: jansvoboda11, vsapsai, dexonsmith. bruno added a comment. Thanks for working on this, comments inline. @vsapsai @jansvoboda11 @dexonsmith any headermap related concerns on your side? Comment at: clang/lib/Lex/HeaderSearch.cpp:445 +return None;

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

andrewjcg added a comment. Hmm, I can't repro the module test failures locally. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930 ___ cfe-commits mailing list

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

andrewjcg updated this revision to Diff 351195. andrewjcg added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930 Files: clang/lib/Lex/HeaderSearch.cpp

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

andrewjcg updated this revision to Diff 350767. andrewjcg added a comment. lint Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103930/new/ https://reviews.llvm.org/D103930 Files: clang/lib/Lex/HeaderSearch.cpp

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

andrewjcg added inline comments. Comment at: clang/test/Modules/implicit-module-header-maps.cpp:27 +#define FOO +#include "Before/Mapping.h" This include will fail if modules weren't used. The include name itself doesn't exist and relies on the header map to

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

andrewjcg added a comment. We were hitting this in our build environment when mixing header maps with clang module maps, where the use of the former would prevent properly associated an included header with it's module via the module map. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D103930: [clang][HeaderSearch] Fix implicit module when using header maps

andrewjcg created this revision. Herald added a subscriber: wenlei. andrewjcg updated this revision to Diff 350757. andrewjcg added a comment. andrewjcg edited the summary of this revision. andrewjcg added reviewers: bruno, rsmith. andrewjcg published this revision for review. Herald added a