-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[SemaCXX] Fixed defaulted equality operator deletion issue (#97087)[SemaCXX] Fix defaulted equality operator being incorrectly deleted when using namespace (llvm#97087) #166996
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang Author: Muvvala Sairam Suhas (suhas-1012) ChangesSummaryFixed an issue where defaulted equality comparison operators ( Changes Made
ExplanationPreviously, Clang sometimes performed lookup for defaulted comparison operators without a valid scope, which caused it to miss operators introduced via Fixes: llvm/llvm-project#97087 Full diff: https://github.com/llvm/llvm-project/pull/166996.diff 3 Files Affected:
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index d41ab126c426f..2331774804c89 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -8262,6 +8262,8 @@ class DefaultedComparisonAnalyzer
/// resolution [...]
CandidateSet.exclude(FD);
+ auto &Fns = this->Fns;
+
if (Args[0]->getType()->isOverloadableType())
S.LookupOverloadedBinOp(CandidateSet, OO, Fns, Args);
else
@@ -8913,6 +8915,9 @@ bool Sema::CheckExplicitlyDefaultedComparison(Scope *S, FunctionDecl *FD,
// Perform any unqualified lookups we're going to need to default this
// function.
+ if (!S)
+ S = getScopeForContext(FD->getLexicalDeclContext());
+
if (S) {
UnresolvedSet<32> Operators;
lookupOperatorsForDefaultedComparison(*this, S, Operators,
diff --git a/clang/test/SemaCXX/default-op-notusing-namespace.cpp b/clang/test/SemaCXX/default-op-notusing-namespace.cpp
new file mode 100644
index 0000000000000..48515fb28366f
--- /dev/null
+++ b/clang/test/SemaCXX/default-op-notusing-namespace.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s
+
+struct X {};
+namespace NS {
+ bool operator==(X, X);
+}
+
+struct Y {
+ X x;
+ friend bool operator==(Y, Y);
+};
+
+// There is no `using namespace NS;` here, so this operator==
+// will be implicitly deleted due to missing viable operator== for X.
+bool operator==(Y, Y) = default;
+// expected-error@-1 {{defaulting this equality comparison operator would delete it after its first declaration}}
+// expected-note@9 {{defaulted 'operator==' is implicitly deleted because there is no viable 'operator==' for member 'x'}}
diff --git a/clang/test/SemaCXX/default-op-using-namespace.cpp b/clang/test/SemaCXX/default-op-using-namespace.cpp
new file mode 100644
index 0000000000000..89003ae0a42ca
--- /dev/null
+++ b/clang/test/SemaCXX/default-op-using-namespace.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++20 %s
+// expected-no-diagnostics
+
+struct X {};
+namespace NS {
+ bool operator==(X, X);
+}
+using namespace NS;
+struct Y {
+ X x;
+ friend bool operator==(Y, Y);
+};
+bool operator==(Y, Y) = default;
\ No newline at end of file
|
|
Please let me know if any more test cases or changes are to be done. |
| /// resolution [...] | ||
| CandidateSet.exclude(FD); | ||
|
|
||
| auto &Fns = this->Fns; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has 2 problems: First, it is horrifyingly subtle (with the shadowing). Second, you have to use the type here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this even do? Before this line, Fns is an lvalue equivalent to this->Fns; after this line, Fns is ... an lvalue that aliases this->Fns?
|
|
||
| // Perform any unqualified lookups we're going to need to default this | ||
| // function. | ||
| if (!S) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't align right with the comment. This probably needs a comment itself as well.
| X x; | ||
| friend bool operator==(Y, Y); | ||
| }; | ||
| bool operator==(Y, Y) = default; No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
newline needed here.
| @@ -0,0 +1,17 @@ | |||
| // RUN: %clang_cc1 -std=c++20 -fsyntax-only -verify %s | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two should just be in the same test file, there is no reason to have two separate tests for this. Perhaps find an existing test file for this to live in?
|
|
||
| // Perform any unqualified lookups we're going to need to default this | ||
| // function. | ||
| if (!S) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on @zygoloid comment:
here it looks like the fix is the pass Scope instead of nullptr here:
llvm-project/clang/lib/Sema/SemaDeclCXX.cpp
Line 17990 in 4c63672
| if (CheckExplicitlyDefaultedComparison(nullptr, FD, DefKind.asComparison())) |
Did you try this?
Summary
Fixed an issue where defaulted equality comparison operators (
operator==) were incorrectly treated as implicitly deleted when the requiredoperator==for a member subobject (such asXinsideY) was only made visible through ausing namespacedirective.Changes Made
clang/lib/Sema/SemaDeclCXX.cppto ensure that unqualified lookup correctly accounts forusing namespacedirectives when generating defaulted comparison operators.clang/test/SemaCXX/:default-op-using-namespace.cpp– verifies that the code now compiles successfully and no longer treats the operator as implicitly deleted whenusing namespacemakes it visible.default-op-notusing-namespace.cpp– verifies that the operator remains correctly deleted when nousing namespacedirective is present.Explanation
Previously, Clang sometimes performed lookup for defaulted comparison operators without a valid scope, which caused it to miss operators introduced via
using namespacedirectives.The fix ensures that when the lookup
Scope*isnullptr, Clang now explicitly retrieves the appropriate lexical scope (viagetScopeForContext(FD->getLexicalDeclContext())) instead of ignoring the case.This change allows unqualified lookup to correctly include
using namespacedirectives and ensures consistent behavior during operator synthesis.Fixes: llvm/llvm-project#97087