[PATCH] D120397: [C++20] [Modules] Make the linkage consistent for template and its specialization

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/test/CodeGenCXX/inconsistent-export-template.cpp:8
+
+// CHECK: void @_Z1fIiEvv
+template <>
----------------
The mangled name should contain module name if D118352 is ready.


================
Comment at: clang/test/Modules/inconsist-export-template.cpp:1-2
+// RUN: %clang_cc1 -std=c++20 %s -fsyntax-only -verify
+// expected-no-diagnostics
+export module m;
----------------
urnathan wrote:
> dblaikie wrote:
> > This test doesn't appear to test anything - it verifies that this file produces no diagnostics, but not that it has any other/particular behavior.
> > 
> > Should this be testing codegen to verify that the correct linkage was used on the resulting IR functions?
> > 
> > Are there other ways of observing the particular language-level linkage of the entities to confirm it's as expected?
> yes, this should be a codegen test (clang/test/CodeGenCXX/Modules/cxx20-$something.cpp?
The compiler would crash at the test before the patch landed. So I send the patch here to show that the test wouldn't cause the compiler crash.


================
Comment at: clang/test/Modules/inconsist-export-template.cpp:19-23
+// FIXME: We should reject following specialization,
+// since it tries to export a name which is already introduced.
+export template <>
+void f1<int>() {
+
----------------
urnathan wrote:
> I don't think we should be testing for ill-formed code here.  We want to verify that explicit instantiations, explicit specializations and implicit instantiations all get the expected linkage -- both external linkage on exported entities, module linkage on non-exported module-purview entities.
I think it could add an `expected-error` once we complete the check in compiler so I added the FIXME here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120397/new/

https://reviews.llvm.org/D120397