ChuanqiXu added a comment. In D113545#3598412 <https://reviews.llvm.org/D113545#3598412>, @iains wrote:
> As for review, I can try to pick up the "nits" but not sure that I know the > instantiation sub=system too well, so it would be better if @rsmith could > cast an eye over those parts. Yeah, @rsmith should be the best one to review this. But as you can find, this revision is opened for months... and the revision blocks many uses of C++20 modules. I really hope someone could push this forward... This was tested before internally. > let's discuss the 10.6 example first (I'd guess 10.7 cases will need to be > reviewed too) > > // RUN: rm -rf %t > // RUN: split-file %s %t > // RUN: cd %t > > // RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-header-unit-header > std-10-6-ex1-decl.h \ > // RUN: -o decl.pcm > > // RUN: %clang_cc1 -std=c++20 -emit-header-unit -xc++-header-unit-header > std-10-6-ex1-defn.h \ > // RUN: -o defn.pcm > > // RUN: %clang_cc1 -std=c++20 -emit-module-interface std-10-6-ex1-stuff.cpp > \ > // RUN: -o stuff.pcm > > // RUN: %clang_cc1 -std=c++20 -emit-module-interface std-10-6-ex1-M1.cpp \ > // RUN: -fmodule-file=stuff.pcm -o M1.pcm -fmodule-file=defn.pcm > > // RUN: %clang_cc1 -std=c++20 -emit-module-interface std-10-6-ex1-M2.cpp \ > // RUN: -fmodule-file=stuff.pcm -o M2.pcm -fmodule-file=decl.pcm > > // RUN: %clang_cc1 -std=c++20 std-10-6-ex1-use.cpp \ > // RUN: -fmodule-file=M1.pcm -fmodule-file=M2.pcm -fsyntax-only > > //--- std-10-6-ex1-decl.h > struct X; > > //--- std-10-6-ex1-defn.h > struct X {}; > > //--- std-10-6-ex1-stuff.cpp > export module stuff; > export template<typename T, typename U> void foo(T, U u) { auto v = u; } > export template<typename T, typename U> void bar(T, U u) { auto v = *u; } > > //--- std-10-6-ex1-M1.cpp > export module M1; > import "std-10-6-ex1-defn.h"; // provides struct X {}; > import stuff; > > export template<typename T> void f(T t) { > X x; > foo(t, x); > } > > //--- std-10-6-ex1-M2.cpp > export module M2; > import "std-10-6-ex1-decl.h"; // provides struct X; (not a definition) > > import stuff; > export template<typename T> void g(T t) { > X *x; > bar(t, x); > } > > //--- std-10-6-ex1-use.cpp > import M1; > import M2; > > void test() { > f(0); > g(0); > } Thanks for reporting this. I've add the tests. Then my implementation would complain that the definition of ''X' is not reachable when it tries to instantiate `g(T)`. But from my personal reading to [module.context]p2: > During the implicit instantiation of a template whose point of instantiation > is specified as that of an enclosing specialization ([temp.point]), the > instantiation context is the union of the instantiation context of the > enclosing specialization and, if the template is defined in a module > interface unit of a module M and the point of instantiation is not in a > module interface unit of M, the point at the end of the declaration-seq of > the primary module interface unit of M (prior to the private-module-fragment, > if any). I feel the diagnostic might make sense. Since the template function 'g(T)' is defined in a module interface unit. And the point of instantiation is not in a module interface unit of that module. So the point should at the end of the primary module interface unit M2 <https://reviews.llvm.org/M2>. So the definition of X shouldn't be reachable here. So the diagnostic makes sense. I'll send this one to WG21. > as suspected, we also have a missing diagnostic in 10.7 ex1 Yes, this was added before at clang/test/CXX/module/module.reach/ex1.cpp. And I add a FIXME there: // FIXME: We should emit an error for unreachable definition of B. void g() { f(); } I'll file an issue for it once this landed. BTW, the compiler would behave properly if: //--- M.cppm export module M; export import :A; import :B; export B foo(); export void f(B b = B()); //--- Use.cpp export module X; import M; void g() { f(foo()); } // complains successfully This looks even more straight forward. > (I think we might also have some issues with poor diagnostics for items in > PMF (saying that an import is required before use, but of course that import > is impossible - e.g. 10.5) Yes, there are many problems. And as the title said, this is an initial implementation and these problems are not regression bugs so I feel like it should be good to fix them in later versions. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113545/new/ https://reviews.llvm.org/D113545 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits