[clang] Fix overload resolution ranking of inherited constructors #132830
Conversation
Take parameter types of candidate constructors into account when deciding whether to apply the tiebreaker.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (offsetof) ChangesTake parameter types of candidate constructors into account when deciding whether to apply the tiebreaker. Fixes #121331 Full diff: https://github.com/llvm/llvm-project/pull/132830.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index f919b66dd0e41..a3d882312056c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -358,6 +358,9 @@ Bug Fixes to C++ Support
- Fixed a Clang regression in C++20 mode where unresolved dependent call expressions were created inside non-dependent contexts (#GH122892)
- Clang now emits the ``-Wunused-variable`` warning when some structured bindings are unused
and the ``[[maybe_unused]]`` attribute is not applied. (#GH125810)
+- Overload resolution tiebreaker for inherited constructors is now only
+ applied if their parameters have the same type, as required by the C++
+ standard. (#GH121331)
Bug Fixes to AST Handling
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 6d8006b35dcf4..f35d272b470cb 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -40,6 +40,7 @@
#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/Casting.h"
#include <algorithm>
#include <cassert>
#include <cstddef>
@@ -10724,15 +10725,13 @@ bool clang::isBetterOverloadCandidate(
// -- F1 is a constructor for a class D, F2 is a constructor for a base
// class B of D, and for all arguments the corresponding parameters of
// F1 and F2 have the same type.
- // FIXME: Implement the "all parameters have the same type" check.
- bool Cand1IsInherited =
- isa_and_nonnull<ConstructorUsingShadowDecl>(Cand1.FoundDecl.getDecl());
- bool Cand2IsInherited =
- isa_and_nonnull<ConstructorUsingShadowDecl>(Cand2.FoundDecl.getDecl());
- if (Cand1IsInherited != Cand2IsInherited)
- return Cand2IsInherited;
- else if (Cand1IsInherited) {
- assert(Cand2IsInherited);
+ if (isa_and_nonnull<CXXConstructorDecl>(Cand1.Function) &&
+ isa_and_nonnull<CXXConstructorDecl>(Cand2.Function) &&
+ llvm::equal(Cand1.Function->parameters().take_front(NumArgs),
+ Cand2.Function->parameters().take_front(NumArgs),
+ [&](ParmVarDecl *P1, ParmVarDecl *P2) {
+ return S.Context.hasSameUnqualifiedType(P1->getType(), P2->getType());
+ })) {
auto *Cand1Class = cast<CXXRecordDecl>(Cand1.Function->getDeclContext());
auto *Cand2Class = cast<CXXRecordDecl>(Cand2.Function->getDeclContext());
if (Cand1Class->isDerivedFrom(Cand2Class))
diff --git a/clang/test/CXX/drs/cwg22xx.cpp b/clang/test/CXX/drs/cwg22xx.cpp
index 8c8ad9f7f74ee..43bab3da171e4 100644
--- a/clang/test/CXX/drs/cwg22xx.cpp
+++ b/clang/test/CXX/drs/cwg22xx.cpp
@@ -169,6 +169,20 @@ B b;
// since-cxx11-error@-1 {{call to implicitly-deleted default constructor of 'B'}}
// since-cxx11-note@#cwg2273-B {{default constructor of 'B' is implicitly deleted because base class 'A' has a deleted default constructor}}
// since-cxx11-note@#cwg2273-A {{'A' has been explicitly marked deleted here}}
+
+struct X {
+ X(float); // since-cxx11-note {{candidate inherited constructor}}
+ X(void*, int = 0) = delete;
+};
+
+struct Y : X {
+ using X::X; // since-cxx11-note {{constructor from base class 'X' inherited here}}
+ Y(double); // since-cxx11-note {{candidate constructor}}
+ Y(void* const, long = 1);
+};
+
+Y y = 1; // since-cxx11-error {{conversion from 'int' to 'Y' is ambiguous}}
+Y z = nullptr;
#endif
} // namespace cwg2273
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
clang/lib/Sema/SemaOverload.cpp
Outdated
| @@ -40,6 +40,7 @@ | |||
| #include "llvm/ADT/ScopeExit.h" | |||
| #include "llvm/ADT/SmallPtrSet.h" | |||
| #include "llvm/ADT/SmallVector.h" | |||
| #include "llvm/Support/Casting.h" | |||
There was a problem hiding this comment.
I don;t think you need to add this include.
| }; | ||
|
|
||
| Y y = 1; // since-cxx11-error {{conversion from 'int' to 'Y' is ambiguous}} | ||
| Y z = nullptr; |
There was a problem hiding this comment.
The wording says "same type", but here you have two constructors, where their first parameter differs in cv-qualification. I suspect this case falls under the usual "exact match is better than qualification conversion", and doesn't need this tie-breaker, so this case is not related to CWG2273
There was a problem hiding this comment.
Since the const here is at the top level, it is ignored when forming the function type.
The FIXME in the test for CWG2277 is not correct: the relevant tiebreaker applies only to constructors. Removed it and marked both 2273 and 2277 as "Clang 21".
There was a problem hiding this comment.
Since the const here is at the top level, it is ignored when forming the function type.
Formally speaking, what matters here is that adjusted parameters types form parameter-type-list, which overload resolution seems to be concerned with (https://eel.is/c++draft/over#match.general-1, even if I don't really trust this wording). But you are correct.
Then, if hasSameUnqualifiedType aspect of you implementation is supposed to take care of that, I think you need to reconsider, because stripping top-level cv-qualifiers in not the only adjustment that happens (https://eel.is/c++draft/dcl.fct#4). I'd like to see additional tests with parameters of array and function types.
Also, I think it would be worth leaving a comment that default arguments do not contribute to the set of arguments that [over.match.best] considers. (Instead, parameters which have default arguments are discarded per https://eel.is/c++draft/over#match.viable-2.3.sentence-2)
There was a problem hiding this comment.
Added tests for array/function parameters.
The wording related to default arguments is already quoted elsewhere in the code, I'm not sure there's a need to repeat it. (Or did you mean it should be mentioned in the test case?)
| Y(void* const, long = 1); | ||
| using IA = int[42]; | ||
| Y(int, const IA); | ||
| Y(unsigned, int (*)(void)); |
There was a problem hiding this comment.
I'd appreciate other test cases of:
Y(int n, const int[n]);
Y(unsigned, const int (*)(void));
where the types are not the same. (The VLA test may have to live somewhere outside of the DR testing because that's an extension.) Speaking of extensions:
struct X {
X(int) = delete;
};
struct Y : X {
using X::X;
Y(_Atomic int);
};
Y y = 1;
Those have different unqualified types because _Atomic is not stripped as a qualifier because it's Special™.
There was a problem hiding this comment.
Added more test cases.
| llvm::equal(Cand1.Function->parameters().take_front(NumArgs), | ||
| Cand2.Function->parameters().take_front(NumArgs), | ||
| [&](ParmVarDecl *P1, ParmVarDecl *P2) { | ||
| return S.Context.hasSameUnqualifiedType(P1->getType(), |
There was a problem hiding this comment.
We should have a test case for the basic situation:
struct X {
X(int) = delete;
};
struct Y : X {
using X::X; // #cwg2273-Y1
Y(const int);
};
Y y = 1; // This should be accepted
I believe that case will work with your patch, but it would be interesting to swap hasSameUnqualifiedType() with hasSameType() in your implementation and see if it still works. If it does, using hasSameType() will reduce questions about why we're stripping qualifiers when the standard doesn't say to do so in the quoted text. If it does not work, then it would be good to add a comment explaining why you're stripping qualifiers despite the standards quote not saying to do so.
There was a problem hiding this comment.
We need to ignore cv-qualifiers here because their deletion, unlike other parameter transformations, only affects the function's parameter-type-list but not the parameter declaration itself. I added a comment.
Take parameter types of candidate constructors into account when deciding whether to apply the tiebreaker.
Fixes #121331