[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

ChuanqiXu added inline comments. Comment at: clang/lib/Lex/ModuleMap.cpp:935 + // with any legal user-defined module name). + StringRef IName = ".ImplementationUnit"; + assert(!Modules[IName] && "multiple implementation units?"); iains wrote: > ChuanqiXu

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

iains marked an inline comment as done. iains added inline comments. Comment at: clang/lib/Lex/ModuleMap.cpp:935 + // with any legal user-defined module name). + StringRef IName = ".ImplementationUnit"; + assert(!Modules[IName] && "multiple implementation units?");

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

This revision was automatically updated to reflect the committed changes. Closed by commit rG6e4f870a21e3: re-land [C++20][Modules] Introduce an implementation module. (authored by iains). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126959/new/

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

ChuanqiXu accepted this revision. ChuanqiXu added inline comments. Comment at: clang/lib/Lex/ModuleMap.cpp:935 + // with any legal user-defined module name). + StringRef IName = ".ImplementationUnit"; + assert(!Modules[IName] && "multiple implementation units?");

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

iains updated this revision to Diff 509014. iains added a comment. rebased, retested. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126959/new/ https://reviews.llvm.org/D126959 Files: clang/include/clang/Basic/Module.h

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

iains added a comment. It took me a while to get my local macOS based devt. environment to reproduce the problem - but it was as expected; the implementation module was unowned and nothing was deleting it. unless CI throws up anything untoward, I plan to re-land this tomorrow (since there is

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

iains updated this revision to Diff 508662. iains added a comment. rebased, reworked to have the module owned. The implementation module needs to be owned by the mpodul map so that it is released when done. Reworked the comments and assert to check the main file presence. Repository: rG

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

iains reopened this revision. iains added a comment. This revision is now accepted and ready to land. I had a hunch that the issue was the non-ownership of the implementation module. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126959/new/

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

hctim added a comment. Hi, yep, I've reverted upstream. If you could please also integrate https://reviews.llvm.org/rG8c7c1f11ffaacf762e612c65440fd2cbb58ee426 in the relanding, that would be great. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

bjope added a comment. In D126959#4222854 , @iains wrote: > In D126959#4222637 , @bjope wrote: > >> This seem to case problems when building with asan enabled >> (LLVM_USE_SANITIZER='Address'): > >

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

iains added a comment. In D126959#4222637 , @bjope wrote: > This seem to case problems when building with asan enabled > (LLVM_USE_SANITIZER='Address'): investigating... do you need the patch reverted? Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

bjope added a comment. This seem to case problems when building with asan enabled (LLVM_USE_SANITIZER='Address'): Failed Tests (24): Clang :: CXX/basic/basic.link/p2.cpp Clang :: CXX/basic/basic.lookup/basic.lookup.argdep/p5-ex2.cpp Clang ::

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. iains marked an inline comment as done. Closed by commit rGc6e9823724ef: [C++20][Modules] Introduce an implementation module. (authored by iains). Repository: rG

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

ChuanqiXu added a comment. LGTM. Thanks. Comment at: clang/include/clang/Sema/Sema.h:2278 + + /// For an interface unit, this is the implicitly imported interface unit. + clang::Module *ThePrimaryInterface = nullptr; Comment at:

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

iains marked an inline comment as done. iains added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:1672-1677 + // A module implementation unit has visibility of the decls in its implicitly + // imported interface. + if (getLangOpts().CPlusPlusModules && NewM && OldM

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

iains updated this revision to Diff 507535. iains marked 12 inline comments as done. iains edited the summary of this revision. iains added a comment. rebased, addressed review comments, and amended the description. the patch has been repurposed to provide a mechanism to track implementation TUs

Re: [PATCH] D126959: [C++20][Modules] Introduce an implementation module.

Hi Chuanqi > On 17 Mar 2023, at 02:49, Chuanqi Xu via Phabricator > wrote: > > ChuanqiXu added a comment. > > (I don't why I can't send emails in the original mail address. It told me > that your email addressed is not recorded by MX, while I don't know what is > MX. So I tried to reply

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

ChuanqiXu added a comment. > The lookup problems might be considered to be quite a serious bug, and so we > should consider possible back porting and try to avoid making large changes > in theses patches (once that problem is solved, then we can refactor, I > think). I agree it is important

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

iains marked 10 inline comments as done. iains added a comment. General comments (at least my opinion). 1. One intention of this patch is to make Implementation Module Units regular (i.e. they should behave the same as any other module unit). So I would tend to avoid changes that make this

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

ChuanqiXu added a comment. I don't remember clearly if we have a test for the initializer in the implementation unit. Ignore this if we already have one. Comment at: clang/include/clang/Basic/Module.h:137 ImplicitGlobalModuleFragment, }; We may be

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

ChuanqiXu added a comment. (I don't why I can't send emails in the original mail address. It told me that your email addressed is not recorded by MX, while I don't know what is MX. So I tried to reply your email here) This FIXME was added 6 years ago for Modules TS.

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

iains added a comment. This was originally created (and @ChuanqiXu approved) for the work on module initialisers. I did not apply it then, since it was possible to determine the correct state for the initialisers without it. However, now we are trying to handle some of the details of

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

iains updated this revision to Diff 505759. iains added a comment. rebased, and reworked for changes in main. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126959/new/ https://reviews.llvm.org/D126959 Files: clang/include/clang/Basic/Module.h

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

iains updated this revision to Diff 434774. iains added a comment. rebased and tidied (still changes planned). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126959/new/ https://reviews.llvm.org/D126959 Files: clang/include/clang/Basic/Module.h

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

iains planned changes to this revision. iains added a comment. thanks for the reviews, I need to figure out why the implementation passes clang tests but gives fails for clangd. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126959/new/

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

ChuanqiXu added a comment. In D126959#3556115 , @iains wrote: > In D126959#3556074 , @tahonermann > wrote: > >>> Implementation modules are never serialized (-emit-module-interface for an >>> implementation

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

ChuanqiXu added inline comments. Comment at: clang/lib/Lex/ModuleMap.cpp:910 + return Result; +} + The implementation looks similar to `createModuleForInterfaceUnit` really. It looks better to refactor it to `createModuleUnits` (or

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

iains added a comment. In D126959#3556074 , @tahonermann wrote: >> Implementation modules are never serialized (-emit-module-interface for an >> implementation unit is diagnosed and rejected). > > Never? Or just not via -emit-module-interface? I would

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

tahonermann added a comment. > Implementation modules are never serialized (-emit-module-interface for an > implementation unit is diagnosed and rejected). Never? Or just not via -emit-module-interface? I would expect to be able to serialize via -emit-ast. Repository: rG LLVM Github

[PATCH] D126959: [C++20][Modules] Introduce an implementation module.

iains created this revision. Herald added a project: All. iains added reviewers: urnathan, ChuanqiXu. iains added a subscriber: clang-modules. iains published this revision for review. iains added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. we need to