[PATCH] D133088: [Clang] Fix wrong diagnostic for scope identifier with internal linkage

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5904-5905
 def err_block_extern_cant_init : Error<
-  "'extern' variable cannot have an initializer">;
+  "declaration of block scope identifier with %select{external|internal}0 "
+  "linkage shall have no initializer">;
 def warn_extern_init : Warning<"'extern' variable has an initializer">,
----------------
I don't think we want to try to call out external vs internal linkage here, more on why below. Also, we don't usually use "shall" in diagnostic wording.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:12774-12784
+  // C99 6.7.8p5. C++ has no such restriction, but that is a defect.
   if (VDecl->isLocalVarDecl() && VDecl->hasExternalStorage()) {
-    // C99 6.7.8p5. C++ has no such restriction, but that is a defect.
-    Diag(VDecl->getLocation(), diag::err_block_extern_cant_init);
+    unsigned LinkageKind = /*external*/ 0;
+    // C2x 6.7.10p6.
+    if (VDecl->getFormalLinkage() == InternalLinkage)
+      LinkageKind = /*internal*/ 1;
+
----------------



================
Comment at: clang/lib/Sema/SemaDecl.cpp:12776-12778
+    unsigned LinkageKind = /*external*/ 0;
+    // C2x 6.7.10p6.
+    if (VDecl->getFormalLinkage() == InternalLinkage)
----------------



================
Comment at: clang/test/Sema/err-decl-block-extern-no-init.c:6
+{
+    extern int x = 1; // expected-error {{declaration of block scope identifier with internal linkage shall have no initializer}}
+}
----------------
This example is why I think we should reword the diagnostic. As it stands, this diagnostic wording would be baffling to most users because they'd go "whaddya mean *INTERNAL* linkage, I said `extern` for a reason!" So using a diagnostic wording of "with linkage" sidesteps that -- what the linkage is doesn't matter for fixing the code, it's the initializer that's the issue.

Also, I don't know what it means for a block scope variable to have *internal* linkage instead of *no* linkage (and I'm not certain it's possible to observe the difference).

That said, I think this example should have a *second* diagnostic, which is a warning, letting the user know that the actual linkage does not match the specified linkage. (This should be done in a separate patch.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133088