fwolff created this revision.
fwolff added reviewers: rsmith, cor3ntin, mizvekov.
fwolff added a project: clang.
Herald added a project: All.
fwolff requested review of this revision.
Herald added a subscriber: cfe-commits.

Fixes #50794 <https://github.com/llvm/llvm-project/issues/50794>. Reopening an 
inline namespace without the `inline` keyword yields a warning:

  inline namespace abc {}
  namespace abc {}
  namespace abc {}

  <source>:2:11: warning: inline namespace reopened as a non-inline namespace 
[-Winline-namespace-reopened-noninline]
  namespace abc {}
            ^
  inline 
  <source>:1:18: note: previous definition is here
  inline namespace abc {}
                   ^
  <source>:3:11: warning: inline namespace reopened as a non-inline namespace 
[-Winline-namespace-reopened-noninline]
  namespace abc {}
            ^
  inline 
  <source>:2:11: note: previous definition is here
  namespace abc {}
            ^

But you'll notice that the second "previous definition is here" is actually not 
helpful, because this previous definition isn't explicitly marked `inline`. I 
think that the note should point to the first definition instead, which must 
always <https://eel.is/c++draft/namespace.def#general-4> have the `inline` if 
the namespace is supposed to be an inline namespace.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122278

Files:
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/Parser/cxx2a-inline-nested-namespace-definition.cpp
  clang/test/SemaCXX/warn-inline-namespace-reopened-twice.cpp


Index: clang/test/SemaCXX/warn-inline-namespace-reopened-twice.cpp
===================================================================
--- /dev/null
+++ clang/test/SemaCXX/warn-inline-namespace-reopened-twice.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -fsyntax-only -Wall -verify -std=c++11 %s
+
+// Regression test for #50794.
+
+// expected-note@+1 2 {{previous definition is here}}
+inline namespace X {}
+
+namespace X {} // expected-warning {{inline namespace reopened as a non-inline 
namespace}}
+namespace X {} // expected-warning {{inline namespace reopened as a non-inline 
namespace}}
Index: clang/test/Parser/cxx2a-inline-nested-namespace-definition.cpp
===================================================================
--- clang/test/Parser/cxx2a-inline-nested-namespace-definition.cpp
+++ clang/test/Parser/cxx2a-inline-nested-namespace-definition.cpp
@@ -17,7 +17,7 @@
 // expected-warning@+2 {{inline nested namespace definition is incompatible 
with C++ standards before C++20}}
 #endif
 namespace valid1::valid2::inline valid3::inline valid4::valid5 {}
-// expected-note@-1 2 {{previous definition is here}}
+// expected-note@-1 4 {{previous definition is here}}
 
 #if __cplusplus <= 201402L
 // expected-warning@+3 {{nested namespace definition is a C++17 extension; 
define each namespace separately}}
@@ -34,7 +34,6 @@
 // expected-warning@+2 {{inline nested namespace definition is incompatible 
with C++ standards before C++20}}
 #endif
 namespace valid1::valid2::inline valid3::inline valid4::valid5 {}
-// expected-note@-1 2 {{previous definition is here}}
 
 namespace valid1 {
 namespace valid2 {
Index: clang/lib/Sema/SemaDeclCXX.cpp
===================================================================
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -11059,6 +11059,12 @@
                                             NamespaceDecl *PrevNS) {
   assert(*IsInline != PrevNS->isInline());
 
+  if (auto *FirstNS = PrevNS->getFirstDecl())
+    // 'inline' must appear on the original definition, but not necessarily
+    // on all extension definitions, so the note should point to the first
+    // definition to avoid confusion.
+    PrevNS = FirstNS;
+
   if (PrevNS->isInline())
     // The user probably just forgot the 'inline', so suggest that it
     // be added back.


Index: clang/test/SemaCXX/warn-inline-namespace-reopened-twice.cpp
===================================================================
--- /dev/null
+++ clang/test/SemaCXX/warn-inline-namespace-reopened-twice.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -fsyntax-only -Wall -verify -std=c++11 %s
+
+// Regression test for #50794.
+
+// expected-note@+1 2 {{previous definition is here}}
+inline namespace X {}
+
+namespace X {} // expected-warning {{inline namespace reopened as a non-inline namespace}}
+namespace X {} // expected-warning {{inline namespace reopened as a non-inline namespace}}
Index: clang/test/Parser/cxx2a-inline-nested-namespace-definition.cpp
===================================================================
--- clang/test/Parser/cxx2a-inline-nested-namespace-definition.cpp
+++ clang/test/Parser/cxx2a-inline-nested-namespace-definition.cpp
@@ -17,7 +17,7 @@
 // expected-warning@+2 {{inline nested namespace definition is incompatible with C++ standards before C++20}}
 #endif
 namespace valid1::valid2::inline valid3::inline valid4::valid5 {}
-// expected-note@-1 2 {{previous definition is here}}
+// expected-note@-1 4 {{previous definition is here}}
 
 #if __cplusplus <= 201402L
 // expected-warning@+3 {{nested namespace definition is a C++17 extension; define each namespace separately}}
@@ -34,7 +34,6 @@
 // expected-warning@+2 {{inline nested namespace definition is incompatible with C++ standards before C++20}}
 #endif
 namespace valid1::valid2::inline valid3::inline valid4::valid5 {}
-// expected-note@-1 2 {{previous definition is here}}
 
 namespace valid1 {
 namespace valid2 {
Index: clang/lib/Sema/SemaDeclCXX.cpp
===================================================================
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -11059,6 +11059,12 @@
                                             NamespaceDecl *PrevNS) {
   assert(*IsInline != PrevNS->isInline());
 
+  if (auto *FirstNS = PrevNS->getFirstDecl())
+    // 'inline' must appear on the original definition, but not necessarily
+    // on all extension definitions, so the note should point to the first
+    // definition to avoid confusion.
+    PrevNS = FirstNS;
+
   if (PrevNS->isInline())
     // The user probably just forgot the 'inline', so suggest that it
     // be added back.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to