Skip to content

[clang] Fix overload resolution ranking of inherited constructors #132830

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[clang] Fix overload resolution ranking of inherited constructors #132830

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e458a6b
[clang] Fix overload resolution ranking of inherited constructors
offsetof Mar 24, 2025
bd15fc4
fixup! [clang] Fix overload resolution ranking of inherited constructors
offsetof Mar 24, 2025
9eb3b12
fixup! [clang] Fix overload resolution ranking of inherited constructors
offsetof Mar 25, 2025
5a1f4c1
fixup! [clang] Fix overload resolution ranking of inherited constructors
offsetof Mar 31, 2025
531eba3
fixup! [clang] Fix overload resolution ranking of inherited constructors
offsetof Apr 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
^^^^^^^^^^^^^^^^^^^^^^^^^
Expand Down
18 changes: 9 additions & 9 deletions clang/lib/Sema/SemaOverload.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Support/Casting.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don;t think you need to add this include.

#include <algorithm>
#include <cassert>
#include <cstddef>
Expand Down Expand Up @@ -10724,15 +10725,14 @@ 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(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

P2->getType());
})) {
auto *Cand1Class = cast<CXXRecordDecl>(Cand1.Function->getDeclContext());
auto *Cand2Class = cast<CXXRecordDecl>(Cand2.Function->getDeclContext());
if (Cand1Class->isDerivedFrom(Cand2Class))
Expand Down
14 changes: 14 additions & 0 deletions clang/test/CXX/drs/cwg22xx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

@Endilll Endilll Mar 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

#endif
} // namespace cwg2273

Expand Down