frederic-tingaud-sonarsource created this revision.
frederic-tingaud-sonarsource added a reviewer: rnk.
Herald added a project: All.
frederic-tingaud-sonarsource requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Patch https://reviews.llvm.org/D124613 introduced an ADL leakage of friend
functions when in MSVC compatibility mode. It broke some C++ compliant code.
That patch fixes the problem introduced.
The original fix flagged friend function declarations as declarations, to
follow MSVC behavior. The problem it introduced is that this same flag is used
to do ADL and that made friend functions visible at the namespace level instead
of them being visible at the class level only.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D125526
Files:
clang/lib/Sema/SemaLookup.cpp
clang/test/SemaCXX/ms-friend-function-decl.cpp
Index: clang/test/SemaCXX/ms-friend-function-decl.cpp
===================================================================
--- clang/test/SemaCXX/ms-friend-function-decl.cpp
+++ clang/test/SemaCXX/ms-friend-function-decl.cpp
@@ -1,9 +1,5 @@
-// RUN: %clang_cc1 -std=c++03 -fms-compatibility -fsyntax-only -verify %s
-// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify %s
-// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=modern
%s
-#if __cplusplus < 202002L
-// expected-no-diagnostics
-#endif
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only
-verify=expected %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only
-verify=modern,expected %s
namespace ns {
@@ -43,3 +39,43 @@
void funGlob() {
}
+
+// At some point, considering friend declaration as full declarations broke
ADL detection.
+// The following test checks that the friend declaration is only visible
inside the class it's declared in.
+namespace check_adl {
+template <typename T>
+struct A {};
+
+struct NotADLVisible {
+ template <typename T>
+ friend constexpr bool operator==(A<T>, NotADLVisible) noexcept {
+ static_assert(sizeof(T) < 0, "This should never be instantiated");
+ return false;
+ }
+
+ constexpr NotADLVisible(int) {}
+};
+} // namespace check_adl
+
+void shouldNotSeeAnyOperator() {
+ bool b = check_adl::A<int>() == 2; // expected-error {{invalid operands to
binary expression ('check_adl::A<int>' and 'int')}}
+}
+
+// Check that we don't break the case of a function that is both visible to
ADL and a friend
+namespace check_adl_compliant {
+struct A {};
+
+struct B {
+ friend constexpr bool operator==(A, B) noexcept;
+
+ constexpr B(int) {}
+};
+
+constexpr bool operator==(check_adl_compliant::A, check_adl_compliant::B)
noexcept {
+ return false;
+}
+} // namespace check_adl_compliant
+
+void shouldSeeTheOperator() {
+ bool b = check_adl_compliant::A() == 2;
+}
Index: clang/lib/Sema/SemaLookup.cpp
===================================================================
--- clang/lib/Sema/SemaLookup.cpp
+++ clang/lib/Sema/SemaLookup.cpp
@@ -3634,14 +3634,17 @@
bool Visible = false;
for (D = D->getMostRecentDecl(); D;
D = cast_or_null<NamedDecl>(D->getPreviousDecl())) {
- if (D->getIdentifierNamespace() & Decl::IDNS_Ordinary) {
- if (isVisible(D)) {
+ // We check the friend case first because in case of MSVC compatibility
+ // a friend declaration is also flagged as Ordinary but we don't want
+ // it to be visible outside its associated class.
+ if (D->getFriendObjectKind()) {
+ auto *RD = cast<CXXRecordDecl>(D->getLexicalDeclContext());
+ if (AssociatedClasses.count(RD) && isVisible(D)) {
Visible = true;
break;
}
- } else if (D->getFriendObjectKind()) {
- auto *RD = cast<CXXRecordDecl>(D->getLexicalDeclContext());
- if (AssociatedClasses.count(RD) && isVisible(D)) {
+ } else if (D->getIdentifierNamespace() & Decl::IDNS_Ordinary) {
+ if (isVisible(D)) {
Visible = true;
break;
}
Index: clang/test/SemaCXX/ms-friend-function-decl.cpp
===================================================================
--- clang/test/SemaCXX/ms-friend-function-decl.cpp
+++ clang/test/SemaCXX/ms-friend-function-decl.cpp
@@ -1,9 +1,5 @@
-// RUN: %clang_cc1 -std=c++03 -fms-compatibility -fsyntax-only -verify %s
-// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify %s
-// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=modern %s
-#if __cplusplus < 202002L
-// expected-no-diagnostics
-#endif
+// RUN: %clang_cc1 -std=c++17 -fms-compatibility -fsyntax-only -verify=expected %s
+// RUN: %clang_cc1 -std=c++20 -fms-compatibility -fsyntax-only -verify=modern,expected %s
namespace ns {
@@ -43,3 +39,43 @@
void funGlob() {
}
+
+// At some point, considering friend declaration as full declarations broke ADL detection.
+// The following test checks that the friend declaration is only visible inside the class it's declared in.
+namespace check_adl {
+template <typename T>
+struct A {};
+
+struct NotADLVisible {
+ template <typename T>
+ friend constexpr bool operator==(A<T>, NotADLVisible) noexcept {
+ static_assert(sizeof(T) < 0, "This should never be instantiated");
+ return false;
+ }
+
+ constexpr NotADLVisible(int) {}
+};
+} // namespace check_adl
+
+void shouldNotSeeAnyOperator() {
+ bool b = check_adl::A<int>() == 2; // expected-error {{invalid operands to binary expression ('check_adl::A<int>' and 'int')}}
+}
+
+// Check that we don't break the case of a function that is both visible to ADL and a friend
+namespace check_adl_compliant {
+struct A {};
+
+struct B {
+ friend constexpr bool operator==(A, B) noexcept;
+
+ constexpr B(int) {}
+};
+
+constexpr bool operator==(check_adl_compliant::A, check_adl_compliant::B) noexcept {
+ return false;
+}
+} // namespace check_adl_compliant
+
+void shouldSeeTheOperator() {
+ bool b = check_adl_compliant::A() == 2;
+}
Index: clang/lib/Sema/SemaLookup.cpp
===================================================================
--- clang/lib/Sema/SemaLookup.cpp
+++ clang/lib/Sema/SemaLookup.cpp
@@ -3634,14 +3634,17 @@
bool Visible = false;
for (D = D->getMostRecentDecl(); D;
D = cast_or_null<NamedDecl>(D->getPreviousDecl())) {
- if (D->getIdentifierNamespace() & Decl::IDNS_Ordinary) {
- if (isVisible(D)) {
+ // We check the friend case first because in case of MSVC compatibility
+ // a friend declaration is also flagged as Ordinary but we don't want
+ // it to be visible outside its associated class.
+ if (D->getFriendObjectKind()) {
+ auto *RD = cast<CXXRecordDecl>(D->getLexicalDeclContext());
+ if (AssociatedClasses.count(RD) && isVisible(D)) {
Visible = true;
break;
}
- } else if (D->getFriendObjectKind()) {
- auto *RD = cast<CXXRecordDecl>(D->getLexicalDeclContext());
- if (AssociatedClasses.count(RD) && isVisible(D)) {
+ } else if (D->getIdentifierNamespace() & Decl::IDNS_Ordinary) {
+ if (isVisible(D)) {
Visible = true;
break;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
