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

[Iain Sandoe via Phabricator via cfe-commits]

iains marked 2 inline comments as done.
iains added subscribers: vsapsai, dblaikie.
iains added a comment.

So the adjustment to the error message is something I am 50/50 about (IMO it makes some messages more useful, but maybe not needed in others).

Without the change we get 
"cannot export redeclaration 'xxx' here since the previous declaration is not exported"

So, e.g in C++20 10.2 example 6. every case has the same error message (which was what prompted me to make the change).

With the change here we now get:
cannot export redeclaration 'f' here since the previous declaration has internal linkage
cannot export redeclaration 'S' here since the previous declaration has module linkage

which seems maybe to be more helpful to the user in telling them why.

I hope others can weigh in with an opinion here  .. @dblaikie @vsapsai  ?



================
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<
----------------
ChuanqiXu wrote:
> I feel this change is not intended?
the change is intentional, but we should maybe decide if it is more or less helpful to the user.


================
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;
----------------
ChuanqiXu wrote:
> 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.
it is reflected in the summary:

"
This adjusts error messages for exports involving redeclarations in modules to
be more specific about the reason that the decl has been rejected.
"
Iet us discuss whether this is more/less useful outside of the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122119