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