[C] Fix a false-positive with tentative defn compat

[AaronBallman]

C++ has a carve-out that makes a declaration with 'extern' explicitly specified and no initializer be a declaration rather than a definition. We now account for that to silence a diagnostic with:

  extern const int i;
  const int i = 12;

which is valid C++.

Addresses an issue that was brought up via post-commit review.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Aaron Ballman (AaronBallman)

Changes

C++ has a carve-out that makes a declaration with 'extern' explicitly specified and no initializer be a declaration rather than a definition. We now account for that to silence a diagnostic with:

  extern const int i;
  const int i = 12;

which is valid C++.

Addresses an issue that was brought up via post-commit review.

---

Full diff: https://github.com/llvm/llvm-project/pull/139738.diff

2 Files Affected:

• (modified) clang/lib/Sema/SemaDecl.cpp (+10-2)
• (modified) clang/test/Sema/warn-tentative-defn-compat.c (+4-1)

diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 5a45198a7ce02..152f3f340cd50 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -4755,8 +4755,16 @@ void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) {
         return;
     }
   } else {
-    Diag(New->getLocation(), diag::warn_cxx_compat_tentative_definition) << New;
-    Diag(Old->getLocation(), diag::note_previous_declaration);
+    // C++ may not have a tentative definition rule, but it has a different
+    // rule about what constitutes a definition in the first place. See
+    // [basic.def]p2 for details, but the basic idea is: if the old declaration
+    // contains the extern specifier and doesn't have an initializer, it's fine
+    // in C++.
+    if (Old->getStorageClass() != SC_Extern || Old->hasInit()) {
+      Diag(New->getLocation(), diag::warn_cxx_compat_tentative_definition)
+          << New;
+      Diag(Old->getLocation(), diag::note_previous_declaration);
+    }
   }
 
   if (haveIncompatibleLanguageLinkages(Old, New)) {
diff --git a/clang/test/Sema/warn-tentative-defn-compat.c b/clang/test/Sema/warn-tentative-defn-compat.c
index 02f3db99992f1..6e6b70df2cf7b 100644
--- a/clang/test/Sema/warn-tentative-defn-compat.c
+++ b/clang/test/Sema/warn-tentative-defn-compat.c
@@ -20,4 +20,7 @@ int k = 12; // expected-warning {{duplicate declaration of 'k' is invalid in C++
                cxx-error {{redefinition of 'k'}}
 
 // Cannot have two declarations with initializers, that is a redefinition in
-// both C and C++.
+// both C and C++. However, C++ does have a different definition of what makes
+// a declaration a definition.
+extern const int a;
+const int a = 12; // Okay in C and C++

[asmok-g]

I'm afraid I'm not in good position to review this change. I just work on providing reproducers for internal failures at google : )

With that please feel free to merge..

[AaronBallman]

I'm afraid I'm not in good position to review this change. I just work on providing reproducers for internal failures at google : )

Good to know! :-)

With that please feel free to merge..

Thanks, I will once precommit CI starts looking more green.