[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7832-7833
 def err_redeclaration_non_exported : Error <
-  "cannot export redeclaration %0 here since the previous declaration is not "
-  "exported">;
+  "cannot export redeclaration %0 here since the previous declaration "
+  "%select{is not exported|has internal linkage|has module linkage}1">;
 def err_invalid_declarator_global_scope : Error<
----------------
I feel this change is not intended?


================
Comment at: clang/lib/Sema/SemaDecl.cpp:1671-1677
+  auto Lk = Old->getFormalLinkage();
+  int S = 0;
+  if (Lk == Linkage::InternalLinkage)
+    S = 1;
+  else if (Lk == Linkage::ModuleLinkage)
+    S = 2;
+  Diag(New->getLocation(), diag::err_redeclaration_non_exported) << New << S;
----------------
It looks like that this change isn't reflected in the summary. Although this is not bad, I feel it is not good. From the perspective of the user, I feel the new message here is a little bit more confusing.


================
Comment at: clang/lib/Sema/SemaModule.cpp:774-776
+  if (auto UDK = getUnnamedDeclKind(D)) {
     diagExportedUnnamedDecl(S, *UDK, D, BlockStart);
+  }
----------------
Unnecessary change?


================
Comment at: clang/lib/Sema/SemaModule.cpp:783
     // instead.
-    if (ND->getDeclName() && ND->getFormalLinkage() == InternalLinkage) {
+    HasName = !!ND->getDeclName();
+    if (HasName && ND->getFormalLinkage() == InternalLinkage) {
----------------
nit: the general style in clang/llvm I see is to use (bool) conversion.


================
Comment at: clang/lib/Sema/SemaModule.cpp:814-815
+        diagExportedUnnamedDecl(S, UnnamedDeclKind::Namespace, D, BlockStart);
+      else
+        ; // We allow an empty named namespace decl.
+    } else if (DC->getRedeclContext()->isFileContext() && !isa<EnumDecl>(D))
----------------
I think we should remove it. So that the above `if` could be further merged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122119