[PATCH] D113545: [C++20] [Module] Support reachable definition initially/partially

[Chuanqi Xu via Phabricator via cfe-commits]

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