Skip to content

[Sema] Diagnose by-value copy constructors in template instantiations #130866

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

Merged
merged 4 commits into from
Mar 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,7 @@ Bug Fixes to Attribute Support
Bug Fixes to C++ Support
^^^^^^^^^^^^^^^^^^^^^^^^

- Clang now diagnoses copy constructors taking the class by value in template instantiations. (#GH130866)
- Clang is now better at keeping track of friend function template instance contexts. (#GH55509)
- Clang now prints the correct instantiation context for diagnostics suppressed
by template argument deduction.
Expand Down
4 changes: 2 additions & 2 deletions clang/lib/Sema/SemaDeclCXX.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10921,8 +10921,8 @@ void Sema::CheckConstructor(CXXConstructorDecl *Constructor) {
// parameters have default arguments.
if (!Constructor->isInvalidDecl() &&
Constructor->hasOneParamOrDefaultArgs() &&
Constructor->getTemplateSpecializationKind() !=
TSK_ImplicitInstantiation) {
!Constructor->isFunctionTemplateSpecialization()
) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you run clang-format on this? I am surprised it would leave the ) on the line like that but maybe I am off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for catching this, and I am so sorry for the oversight.
I forgot to run clang-format before merging.
Should I submitt a follow-up PR to fix the formatting issues?

Copy link
Contributor

Choose a reason for hiding this comment

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

I missed it too, sorry.

yes, feel free to submit a new pr. thanks

QualType ParamType = Constructor->getParamDecl(0)->getType();
QualType ClassTy = Context.getTagDeclType(ClassDecl);
if (Context.getCanonicalType(ParamType).getUnqualifiedType() == ClassTy) {
Expand Down
29 changes: 29 additions & 0 deletions clang/test/SemaCXX/copy-ctor-template.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// RUN: %clang_cc1 -fsyntax-only -verify %s

template<class T, class V>
struct A{
A(); // expected-note{{candidate constructor not viable: requires 0 arguments, but 1 was provided}}
A(A&); // expected-note{{candidate constructor not viable: expects an lvalue for 1st argument}}
A(A<V, T>); // expected-error{{copy constructor must pass its first argument by reference}}
};

void f() {
A<int, int> a = A<int, int>(); // expected-note{{in instantiation of template class 'A<int, int>'}}
A<int, double> a1 = A<double, int>(); // No error (not a copy constructor)
}

// Test rvalue-to-lvalue conversion in copy constructor
A<int, int> &&a(void);
void g() {
A<int, int> a2 = a(); // expected-error{{no matching constructor}}
}

template<class T, class V>
struct B{
B();
template<class U> B(U); // No error (templated constructor)
};

void h() {
B<int, int> b = B<int, int>(); // should use implicit copy constructor
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test for @hubert-reinterpretcast comment here
#80963 (comment)

Can you add tests for something like
A<int, double> a = A<double, int>(); // not a copy constructor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! I’ll add tests for the A<int, double> case and update the PR shortly.

15 changes: 8 additions & 7 deletions clang/test/SemaTemplate/constructor-template.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ struct X3 {
template<typename T> X3(T);
};

template<> X3::X3(X3); // expected-error{{must pass its first argument by reference}}
template<> X3::X3(X3); // No error (template constructor)

struct X4 {
X4();
Expand Down Expand Up @@ -139,12 +139,12 @@ namespace self_by_value {
template <class T, class U> struct A {
A() {}
A(const A<T,U> &o) {}
A(A<T,T> o) {}
A(A<T,T> o) {} // expected-error{{copy constructor must pass its first argument by reference}}
};

void helper(A<int,float>);

void test1(A<int,int> a) {
void test1(A<int,int> a) { // expected-note{{in instantiation of template class 'self_by_value::A<int, int>'}}
helper(a);
}
void test2() {
Expand All @@ -156,24 +156,25 @@ namespace self_by_value_2 {
template <class T, class U> struct A {
A() {} // precxx17-note {{not viable: requires 0 arguments}}
A(A<T,U> &o) {} // precxx17-note {{not viable: expects an lvalue}}
A(A<T,T> o) {} // precxx17-note {{ignored: instantiation takes its own class type by value}}
A(A<T,T> o) {} // expected-error{{copy constructor must pass its first argument by reference}}
};

void helper_A(A<int,int>); // precxx17-note {{passing argument to parameter here}}
void test_A() {
helper_A(A<int,int>()); // precxx17-error {{no matching constructor}}
helper_A(A<int,int>()); // precxx17-error {{no matching constructor}} \
// expected-note{{in instantiation of template class 'self_by_value_2::A<int, int>'}}
}
}

namespace self_by_value_3 {
template <class T, class U> struct A {
A() {}
A(A<T,U> &o) {}
A(A<T,T> o) {}
A(A<T,T> o) {} // expected-error{{copy constructor must pass its first argument by reference}}
};

void helper_A(A<int,int>);
void test_A(A<int,int> b) {
void test_A(A<int,int> b) { // expected-note{{in instantiation of template class 'self_by_value_3::A<int, int>'}}
helper_A(b);
}
}
Loading