[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

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 rG69350e569dc4: [C++20][Modules][3/8] Initial handling for module partitions. (authored by iains). Repository:

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

iains marked an inline comment as done. iains added inline comments. Comment at: clang/test/Modules/cxx20-multiple-partitions.cpp:48 +#else +#error "no TU set" +#endif jansvoboda11 wrote: > Instead of splitting the file with preprocessor, you could use the > `sp

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

iains updated this revision to Diff 410498. iains added a comment. rebased, update testcases to use split-file. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118586/new/ https://reviews.llvm.org/D118586 Files: clang/include/clang/AST/Decl.h cl

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

jansvoboda11 added inline comments. Comment at: clang/test/Modules/cxx20-multiple-partitions.cpp:48 +#else +#error "no TU set" +#endif Instead of splitting the file with preprocessor, you could use the `split-file` utility (introduced in D83834). Repository:

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

ChuanqiXu added a comment. In D118586#3336933 , @iains wrote: > In D118586#3336892 , @ChuanqiXu > wrote: > >> In D118586#3336865 , @iains wrote: >> >>> In D118586#3336612

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

iains added a comment. In D118586#3336892 , @ChuanqiXu wrote: > In D118586#3336865 , @iains wrote: > >> In D118586#3336612 , @ChuanqiXu >> wrote: >> >>> (May off the patc

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

ChuanqiXu added a comment. In D118586#3336865 , @iains wrote: > In D118586#3336612 , @ChuanqiXu > wrote: > >> (May off the patch) > > I am not sure what you mean here. I mean this comment is not related to the p

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

iains added a comment. In D118586#3336612 , @ChuanqiXu wrote: > (May off the patch) I am not sure what you mean here. > BTW, I know you also work on GCC frontend. I want to consultant what's your > opinion to uniform the command line options between Cl

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

ChuanqiXu added a comment. (May off the patch) BTW, I know you also work on GCC frontend. I want to consultant what's your opinion to uniform the command line options between Clang and GCC. Now it looks totally different. I feel it would be a problem for clang. Since the command line between c

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

iains updated this revision to Diff 410320. iains added a comment. rebased, added a multi-partition testcase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118586/new/ https://reviews.llvm.org/D118586 Files: clang/include/clang/AST/Decl.h clan

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

iains added a comment. In D118586#3335235 , @ChuanqiXu wrote: > In D118586#3335191 , @iains wrote: > >> In D118586#3335167 , @ChuanqiXu >> wrote: >> >>> I think it is hel

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

ChuanqiXu added a comment. In D118586#3335191 , @iains wrote: > In D118586#3335167 , @ChuanqiXu > wrote: > >> I think it is helpful to add some tests from https://reviews.llvm.org/D113972 > > Actually, in respons

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

iains added a comment. In D118586#3335167 , @ChuanqiXu wrote: > I think it is helpful to add some tests from https://reviews.llvm.org/D113972 Actually, in response to your concerns, this morning I wrote a multi-partition test case to add to this (which

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

ChuanqiXu added a comment. I think it is helpful to add some tests from https://reviews.llvm.org/D113972 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118586/new/ https://reviews.llvm.org/D118586 ___ cfe

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

ChuanqiXu accepted this revision. ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:177 + if (IsPartition) { +ModuleName += ":"; +ModuleName += stringFromPath(Partition); urnathan wrote: > iains wrote: > > ChuanqiXu wrote: > > >

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

urnathan added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:177 + if (IsPartition) { +ModuleName += ":"; +ModuleName += stringFromPath(Partition); iains wrote: > ChuanqiXu wrote: > > iains wrote: > > > urnathan wrote: > > > > iains wrote: >

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

iains added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:177 + if (IsPartition) { +ModuleName += ":"; +ModuleName += stringFromPath(Partition); ChuanqiXu wrote: > iains wrote: > > urnathan wrote: > > > iains wrote: > > > > ChuanqiXu wrote:

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:177 + if (IsPartition) { +ModuleName += ":"; +ModuleName += stringFromPath(Partition); iains wrote: > urnathan wrote: > > iains wrote: > > > ChuanqiXu wrote: > > > > I chose '-' i

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

urnathan accepted this revision. urnathan added a comment. This revision is now accepted and ready to land. thanks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118586/new/ https://reviews.llvm.org/D118586 _

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

iains added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:177 + if (IsPartition) { +ModuleName += ":"; +ModuleName += stringFromPath(Partition); urnathan wrote: > iains wrote: > > ChuanqiXu wrote: > > > I chose '-' in my implementation since

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

iains updated this revision to Diff 409614. iains marked 9 inline comments as done. iains added a comment. address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118586/new/ https://reviews.llvm.org/D118586 Files: clang/include/cl

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

urnathan added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:177 + if (IsPartition) { +ModuleName += ":"; +ModuleName += stringFromPath(Partition); iains wrote: > ChuanqiXu wrote: > > I chose '-' in my implementation since here ModuleName re

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

iains marked 7 inline comments as done. iains added a comment. some comments remain to be addressed - this update addressed Nathan's primarily - but also cover some of Chuanqi's Comment at: clang/include/clang/Sema/Sema.h:2989 /// \param ImportLoc The location of the 'impor

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

iains updated this revision to Diff 409552. iains added a comment. address review comments, rebase onto parent patches. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118586/new/ https://reviews.llvm.org/D118586 Files: clang/include/clang/AST/Dec

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:367-368 + // owning named module. + size_t P = NamedMod->Name.find_first_of(":"); + ModuleName = NamedMod->Name.substr(0, P + 1); +} else { ChuanqiXu wrote: > I would li

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

ChuanqiXu added inline comments. Comment at: clang/test/Modules/cxx20-partition-diagnostics-a.cpp:3-5 +// RUN: %clang_cc1 -std=c++20 -S -D TU=1 -x c++ %s -o /dev/null -verify + +// RUN: %clang_cc1 -std=c++20 -S -D TU=2 -x c++ %s -o /dev/null -verify We could use

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:177 + if (IsPartition) { +ModuleName += ":"; +ModuleName += stringFromPath(Partition); I chose '-' in my implementation since here ModuleName refers to the name in filesystem, ri

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

ChuanqiXu added a comment. I think we should add 2 more tests at least: Import a partition a module purview but we failed to find one. And test reflects the successful case. Comment at: clang/include/clang/Sema/Sema.h:2989 /// \param ImportLoc The location of the 'import' k

[PATCH] D118586: [C++20][Modules][3/8] Initial handling for module partitions.

urnathan added inline comments. Comment at: clang/include/clang/Sema/Sema.h:2989 /// \param ImportLoc The location of the 'import' keyword. - /// \param Path The module access path. + /// \param NamePath The module toplevel name as an access path. + /// \param Partition The