[PATCH] D153542: [C++20][Modules] Implement P2615R1 exported specialization diagnostics.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11275-11278
+def err_export_partial_specialization : Error<
+  "partial %select{class|variable}0 specialization %1 cannot be exported">;
+def err_export_explicit_specialization : Error<
+  "explicit specialization %0 cannot be exported">;
----------------
According to #[dcl.pre] in https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2615r1.html, the export for explicit instantiation is also allowed.


================
Comment at: clang/lib/Sema/SemaModule.cpp:831-832
 
+  if (isa<ClassTemplatePartialSpecializationDecl>(D) ||
+      isa<VarTemplatePartialSpecializationDecl>(D)) {
+    // C++20 [module.interface]p1:
----------------
nit


================
Comment at: clang/lib/Sema/SemaModule.cpp:834
+    // C++20 [module.interface]p1:
+    //   [...] shall not declare a partial specialization.
+    int Kind = isa<ClassTemplatePartialSpecializationDecl>(D) ? 0 : 1;
----------------
nit


================
Comment at: clang/lib/Sema/SemaModule.cpp:836
+    int Kind = isa<ClassTemplatePartialSpecializationDecl>(D) ? 0 : 1;
+    auto *ND = dyn_cast<NamedDecl>(D);
+    S.Diag(ND->getLocation(), diag::err_export_partial_specialization)
----------------



================
Comment at: clang/lib/Sema/SemaModule.cpp:845-846
+    //   export-declaration.
+    bool BadExport = isa<ClassTemplateSpecializationDecl>(ND) ||
+                     isa<VarTemplateSpecializationDecl>(ND);
+    if (auto *FD = dyn_cast<FunctionDecl>(D)) {
----------------



================
Comment at: clang/lib/Sema/SemaModule.cpp:847-848
+                     isa<VarTemplateSpecializationDecl>(ND);
+    if (auto *FD = dyn_cast<FunctionDecl>(D)) {
+      if (FD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
+        BadExport = true;
----------------
nit


================
Comment at: clang/lib/Sema/SemaModule.cpp:848-849
+    if (auto *FD = dyn_cast<FunctionDecl>(D)) {
+      if (FD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
+        BadExport = true;
+    } else if (auto *VD = dyn_cast<VarDecl>(D)) {
----------------
Given P2615R1 doesn't allow explicit-instantiation in export block too.


================
Comment at: clang/lib/Sema/SemaModule.cpp:851
+    } else if (auto *VD = dyn_cast<VarDecl>(D)) {
+      if (VD->getTemplateSpecializationKind() == TSK_ExplicitSpecialization)
+        BadExport = true;
----------------
ditto


================
Comment at: clang/test/CXX/temp/temp.explicit/p2-p2615r1.cpp:20
+int C<int>;  // expected-error {{explicit specialization 'C<int>' cannot be exported}}
+
----------------
Let's add another case for explicit instantiation.


================
Comment at: clang/test/Modules/merge-var-template-spec-cxx-modules.cppm:35-36
-};
-export template <> constexpr Int zero<Int> = {0};
-export template <class T> constexpr T* zero<T*> = nullptr;
-
----------------
It should be good enough to remove the "export" in the template specialization.


================
Comment at: clang/test/Modules/pr59780.cppm:18-19
-
-export template<>
-int x<int> = 0;
-
----------------
It should be good to remove the "export" in the template specialization.


================
Comment at: clang/test/Modules/pr60890.cppm:21
 
-export template struct a<double>;
-
----------------
The specialization is meaningful here to test the serializer/deserializer can handle the merge well. It is OK to remove the `export` here in this patch and I'll try to update the tests to make its semantics more clear.


================
Comment at: clang/test/Modules/pr62796.cppm:42-43
-
-    template constexpr unsigned long Cache<10ul>;
 }
 
----------------
In case it is not allowed to **export** the explicit instantiations, we should move it out of the export block instead of removing it.


================
Comment at: clang/test/Modules/template-function-specialization.cpp:42
 
-export template <>
-void foo4<int>() {
----------------
ditto


================
Comment at: clang/unittests/Serialization/VarDeclConstantInitTest.cpp:89
 
-  template constexpr unsigned long Cache<10ul>;
 }
----------------
Same as above, in this case we should move it instead of removing it.


================
Comment at: clang/unittests/Serialization/VarDeclConstantInitTest.cpp:118
 import Fibonacci.Cache;
+template constexpr unsigned long Fibonacci::Cache<10ul>;
         )cpp",
----------------
We don't need this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153542