[Clang] Added explanation why is_trivially_copyable evaluated to false.
#142341
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Shamshura Egor (egorshamshura) ChangesFull diff: https://github.com/llvm/llvm-project/pull/142341.diff 4 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index efc842bb4c42e..748e0720c5ef5 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1764,7 +1764,8 @@ def err_user_defined_msg_constexpr : Error<
// Type traits explanations
def note_unsatisfied_trait : Note<"%0 is not %enum_select<TraitName>{"
- "%TriviallyRelocatable{trivially relocatable}"
+ "%TriviallyRelocatable{trivially relocatable}|"
+ "%TriviallyCopyable{trivially copyable}"
"}1">;
def note_unsatisfied_trait_reason
@@ -1776,6 +1777,8 @@ def note_unsatisfied_trait_reason
"%VBase{has a virtual base %1}|"
"%NRBase{has a non-trivially-relocatable base %1}|"
"%NRField{has a non-trivially-relocatable member %1 of type %2}|"
+ "%NTCBase{has a non-trivially-copyable base %1}|"
+ "%NTCField{has a non-trivially-copyable member %1 of type %2}|"
"%DeletedDtr{has a %select{deleted|user-provided}1 destructor}|"
"%UserProvidedCtr{has a user provided %select{copy|move}1 "
"constructor}|"
diff --git a/clang/lib/Sema/SemaTypeTraits.cpp b/clang/lib/Sema/SemaTypeTraits.cpp
index 7bf3c8eaabf4b..aaa5aff53fbc5 100644
--- a/clang/lib/Sema/SemaTypeTraits.cpp
+++ b/clang/lib/Sema/SemaTypeTraits.cpp
@@ -11,8 +11,10 @@
//===----------------------------------------------------------------------===//
#include "clang/AST/DeclCXX.h"
+#include "clang/AST/Type.h"
#include "clang/Basic/DiagnosticParse.h"
#include "clang/Basic/DiagnosticSema.h"
+#include "clang/Basic/TypeTraits.h"
#include "clang/Sema/EnterExpressionEvaluationContext.h"
#include "clang/Sema/Initialization.h"
#include "clang/Sema/Lookup.h"
@@ -1922,6 +1924,7 @@ static std::optional<TypeTrait> StdNameToTypeTrait(StringRef Name) {
return llvm::StringSwitch<std::optional<TypeTrait>>(Name)
.Case("is_trivially_relocatable",
TypeTrait::UTT_IsCppTriviallyRelocatable)
+ .Case("is_trivially_copyable", TypeTrait::UTT_IsTriviallyCopyable)
.Default(std::nullopt);
}
@@ -2083,6 +2086,97 @@ static void DiagnoseNonTriviallyRelocatableReason(Sema &SemaRef,
SemaRef.Diag(D->getLocation(), diag::note_defined_here) << D;
}
+static void DiagnoseNonTriviallyCopyableReason(Sema &SemaRef,
+ SourceLocation Loc,
+ const CXXRecordDecl *D) {
+ for (const CXXBaseSpecifier &B : D->bases()) {
+ assert(B.getType()->getAsCXXRecordDecl() && "invalid base?");
+ if (B.isVirtual())
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::VBase << B.getType()
+ << B.getSourceRange();
+ if (!B.getType().isTriviallyCopyableType(D->getASTContext())) {
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::NTCBase << B.getType()
+ << B.getSourceRange();
+ }
+ }
+ for (const FieldDecl *Field : D->fields()) {
+ if (!Field->getType().isTriviallyCopyableType(Field->getASTContext()))
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::NTCField << Field
+ << Field->getType() << Field->getSourceRange();
+ }
+ if (D->hasDeletedDestructor())
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::DeletedDtr << 0
+ << D->getDestructor()->getSourceRange();
+
+ if (D->isUnion()) {
+ auto DiagSPM = [&](CXXSpecialMemberKind K, bool Has) {
+ if (Has)
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::UnionWithUserDeclaredSMF << K;
+ };
+ DiagSPM(CXXSpecialMemberKind::CopyConstructor,
+ D->hasUserDeclaredCopyConstructor());
+ DiagSPM(CXXSpecialMemberKind::CopyAssignment,
+ D->hasUserDeclaredCopyAssignment());
+ DiagSPM(CXXSpecialMemberKind::MoveConstructor,
+ D->hasUserDeclaredMoveConstructor());
+ DiagSPM(CXXSpecialMemberKind::MoveAssignment,
+ D->hasUserDeclaredMoveAssignment());
+ return;
+ }
+
+ if (!D->hasSimpleMoveConstructor() && !D->hasSimpleCopyConstructor()) {
+ const auto *Decl = cast<CXXConstructorDecl>(
+ LookupSpecialMemberFromXValue(SemaRef, D, /*Assign=*/false));
+ if (Decl && Decl->isUserProvided())
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::UserProvidedCtr
+ << Decl->isMoveConstructor() << Decl->getSourceRange();
+ }
+ if (!D->hasSimpleMoveAssignment() && !D->hasSimpleCopyAssignment()) {
+ CXXMethodDecl *Decl =
+ LookupSpecialMemberFromXValue(SemaRef, D, /*Assign=*/true);
+ if (Decl && Decl->isUserProvided())
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::UserProvidedAssign
+ << Decl->isMoveAssignmentOperator() << Decl->getSourceRange();
+ }
+ CXXDestructorDecl *Dtr = D->getDestructor();
+ if (Dtr && Dtr->isUserProvided() && !Dtr->isDefaulted())
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::DeletedDtr << 1
+ << Dtr->getSourceRange();
+}
+
+static void DiagnoseNonTriviallyCopyableReason(Sema &SemaRef,
+ SourceLocation Loc, QualType T) {
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait)
+ << T << diag::TraitName::TriviallyCopyable;
+
+ if (T->isReferenceType())
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::Ref;
+
+ T = T.getNonReferenceType();
+
+ if (T.hasNonTrivialObjCLifetime())
+ SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason)
+ << diag::TraitNotSatisfiedReason::HasArcLifetime;
+
+ const CXXRecordDecl *D = T->getAsCXXRecordDecl();
+ if (!D || D->isInvalidDecl())
+ return;
+
+ if (D->hasDefinition())
+ DiagnoseNonTriviallyCopyableReason(SemaRef, Loc, D);
+
+ SemaRef.Diag(D->getLocation(), diag::note_defined_here) << D;
+}
+
void Sema::DiagnoseTypeTraitDetails(const Expr *E) {
E = E->IgnoreParenImpCasts();
if (E->containsErrors())
@@ -2097,6 +2191,9 @@ void Sema::DiagnoseTypeTraitDetails(const Expr *E) {
case UTT_IsCppTriviallyRelocatable:
DiagnoseNonTriviallyRelocatableReason(*this, E->getBeginLoc(), Args[0]);
break;
+ case UTT_IsTriviallyCopyable:
+ DiagnoseNonTriviallyCopyableReason(*this, E->getBeginLoc(), Args[0]);
+ break;
default:
break;
}
diff --git a/clang/test/SemaCXX/type-traits-unsatisfied-diags-std.cpp b/clang/test/SemaCXX/type-traits-unsatisfied-diags-std.cpp
index 90cff1e66000c..498e202e26265 100644
--- a/clang/test/SemaCXX/type-traits-unsatisfied-diags-std.cpp
+++ b/clang/test/SemaCXX/type-traits-unsatisfied-diags-std.cpp
@@ -12,6 +12,14 @@ struct is_trivially_relocatable {
template <typename T>
constexpr bool is_trivially_relocatable_v = __builtin_is_cpp_trivially_relocatable(T);
+
+template <typename T>
+struct is_trivially_copyable {
+ static constexpr bool value = __is_trivially_copyable(T);
+};
+
+template <typename T>
+constexpr bool is_trivially_copyable_v = __is_trivially_copyable(T);
#endif
#ifdef STD2
@@ -25,6 +33,17 @@ using is_trivially_relocatable = __details_is_trivially_relocatable<T>;
template <typename T>
constexpr bool is_trivially_relocatable_v = __builtin_is_cpp_trivially_relocatable(T);
+
+template <typename T>
+struct __details_is_trivially_copyable {
+ static constexpr bool value = __is_trivially_copyable(T);
+};
+
+template <typename T>
+using is_trivially_copyable = __details_is_trivially_copyable<T>;
+
+template <typename T>
+constexpr bool is_trivially_copyable_v = __is_trivially_copyable(T);
#endif
@@ -45,6 +64,15 @@ using is_trivially_relocatable = __details_is_trivially_relocatable<T>;
template <typename T>
constexpr bool is_trivially_relocatable_v = is_trivially_relocatable<T>::value;
+
+template <typename T>
+struct __details_is_trivially_copyable : bool_constant<__is_trivially_copyable(T)> {};
+
+template <typename T>
+using is_trivially_copyable = __details_is_trivially_copyable<T>;
+
+template <typename T>
+constexpr bool is_trivially_copyable_v = is_trivially_copyable<T>::value;
#endif
}
@@ -60,6 +88,18 @@ static_assert(std::is_trivially_relocatable_v<int&>);
// expected-note@-1 {{'int &' is not trivially relocatable}} \
// expected-note@-1 {{because it is a reference type}}
+static_assert(std::is_trivially_copyable<int>::value);
+
+static_assert(std::is_trivially_copyable<int&>::value);
+// expected-error-re@-1 {{static assertion failed due to requirement 'std::{{.*}}is_trivially_copyable<int &>::value'}} \
+// expected-note@-1 {{'int &' is not trivially copyable}} \
+// expected-note@-1 {{because it is a reference type}}
+static_assert(std::is_trivially_copyable_v<int&>);
+// expected-error@-1 {{static assertion failed due to requirement 'std::is_trivially_copyable_v<int &>'}} \
+// expected-note@-1 {{'int &' is not trivially copyable}} \
+// expected-note@-1 {{because it is a reference type}}
+
+
namespace test_namespace {
using namespace std;
static_assert(is_trivially_relocatable<int&>::value);
@@ -70,6 +110,15 @@ namespace test_namespace {
// expected-error@-1 {{static assertion failed due to requirement 'is_trivially_relocatable_v<int &>'}} \
// expected-note@-1 {{'int &' is not trivially relocatable}} \
// expected-note@-1 {{because it is a reference type}}
+
+ static_assert(is_trivially_copyable<int&>::value);
+ // expected-error-re@-1 {{static assertion failed due to requirement '{{.*}}is_trivially_copyable<int &>::value'}} \
+ // expected-note@-1 {{'int &' is not trivially copyable}} \
+ // expected-note@-1 {{because it is a reference type}}
+ static_assert(is_trivially_copyable_v<int&>);
+ // expected-error@-1 {{static assertion failed due to requirement 'is_trivially_copyable_v<int &>'}} \
+ // expected-note@-1 {{'int &' is not trivially copyable}} \
+ // expected-note@-1 {{because it is a reference type}}
}
@@ -82,6 +131,14 @@ concept C = std::is_trivially_relocatable_v<T>; // #concept2
template <C T> void g(); // #cand2
+template <typename T>
+requires std::is_trivially_copyable<T>::value void f2(); // #cand3
+
+template <typename T>
+concept C2 = std::is_trivially_copyable_v<T>; // #concept4
+
+template <C2 T> void g2(); // #cand4
+
void test() {
f<int&>();
// expected-error@-1 {{no matching function for call to 'f'}} \
@@ -97,5 +154,20 @@ void test() {
// expected-note@#concept2 {{because 'std::is_trivially_relocatable_v<int &>' evaluated to false}} \
// expected-note@#concept2 {{'int &' is not trivially relocatable}} \
// expected-note@#concept2 {{because it is a reference type}}
+
+ f2<int&>();
+ // expected-error@-1 {{no matching function for call to 'f2'}} \
+ // expected-note@#cand3 {{candidate template ignored: constraints not satisfied [with T = int &]}} \
+ // expected-note-re@#cand3 {{because '{{.*}}is_trivially_copyable<int &>::value' evaluated to false}} \
+ // expected-note@#cand3 {{'int &' is not trivially copyable}} \
+ // expected-note@#cand3 {{because it is a reference type}}
+
+ g2<int&>();
+ // expected-error@-1 {{no matching function for call to 'g2'}} \
+ // expected-note@#cand4 {{candidate template ignored: constraints not satisfied [with T = int &]}} \
+ // expected-note@#cand4 {{because 'int &' does not satisfy 'C2'}} \
+ // expected-note@#concept4 {{because 'std::is_trivially_copyable_v<int &>' evaluated to false}} \
+ // expected-note@#concept4 {{'int &' is not trivially copyable}} \
+ // expected-note@#concept4 {{because it is a reference type}}
}
}
diff --git a/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp b/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp
index d9cab20f4febd..97510fe2eca9f 100644
--- a/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp
+++ b/clang/test/SemaCXX/type-traits-unsatisfied-diags.cpp
@@ -144,3 +144,83 @@ static_assert(__builtin_is_cpp_trivially_relocatable(U2));
// expected-note@#tr-U2 {{'U2' defined here}}
}
+
+namespace trivially_copyable {
+struct B {
+ virtual ~B();
+};
+struct S : virtual B { // #tc-S
+ S();
+ int & a;
+ const int ci;
+ B & b;
+ B c;
+ ~S();
+};
+static_assert(__is_trivially_copyable(S));
+// expected-error@-1 {{static assertion failed due to requirement '__is_trivially_copyable(trivially_copyable::S)'}} \
+// expected-note@-1 {{'S' is not trivially copyable}} \
+// expected-note@-1 {{because it has a virtual base 'B'}} \
+// expected-note@-1 {{because it has a non-trivially-copyable base 'B'}} \
+// expected-note@-1 {{because it has a non-trivially-copyable member 'c' of type 'B'}} \
+// expected-note@-1 {{because it has a non-trivially-copyable member 'b' of type 'B &'}} \
+// expected-note@-1 {{because it has a non-trivially-copyable member 'a' of type 'int &'}} \
+// expected-note@-1 {{because it has a user-provided destructor}}
+// expected-note@#tc-S {{'S' defined here}}
+
+struct S2 { // #tc-S2
+ S2(S2&&);
+ S2& operator=(const S2&);
+};
+static_assert(__is_trivially_copyable(S2));
+// expected-error@-1 {{static assertion failed due to requirement '__is_trivially_copyable(trivially_copyable::S2)'}} \
+// expected-note@-1 {{'S2' is not trivially copyable}} \
+// expected-note@-1 {{because it has a user provided move constructor}} \
+// expected-note@-1 {{because it has a user provided copy assignment operator}} \
+// expected-note@#tc-S2 {{'S2' defined here}}
+
+struct S3 {
+ ~S3() = delete;
+};
+static_assert(__is_trivially_copyable(S3));
+
+union U { // #tc-U
+ U(const U&);
+ U(U&&);
+ U& operator=(const U&);
+ U& operator=(U&&);
+};
+static_assert(__is_trivially_copyable(U));
+// expected-error@-1 {{static assertion failed due to requirement '__is_trivially_copyable(trivially_copyable::U)'}} \
+// expected-note@-1 {{'U' is not trivially copyable}} \
+// expected-note@-1 {{because it is a union with a user-declared copy constructor}} \
+// expected-note@-1 {{because it is a union with a user-declared copy assignment operator}} \
+// expected-note@-1 {{because it is a union with a user-declared move constructor}} \
+// expected-note@-1 {{because it is a union with a user-declared move assignment operator}}
+// expected-note@#tc-U {{'U' defined here}}
+
+struct S4 { // #tc-S4
+ ~S4();
+ B b;
+};
+static_assert(__is_trivially_copyable(S4));
+// expected-error@-1 {{static assertion failed due to requirement '__is_trivially_copyable(trivially_copyable::S4)'}} \
+// expected-note@-1 {{'S4' is not trivially copyable}} \
+// expected-note@-1 {{because it has a non-trivially-copyable member 'b' of type 'B'}} \
+// expected-note@-1 {{because it has a user-provided destructor}} \
+// expected-note@#tc-S4 {{'S4' defined here}}
+
+union U2 { // #tc-U2
+ U2(const U2&);
+ U2(U2&&);
+ B b;
+};
+static_assert(__is_trivially_copyable(U2));
+// expected-error@-1 {{static assertion failed due to requirement '__is_trivially_copyable(trivially_copyable::U2)'}} \
+// expected-note@-1 {{'U2' is not trivially copyable}} \
+// expected-note@-1 {{because it is a union with a user-declared copy constructor}} \
+// expected-note@-1 {{because it is a union with a user-declared move constructor}} \
+// expected-note@-1 {{because it has a deleted destructor}} \
+// expected-note@-1 {{because it has a non-trivially-copyable member 'b' of type 'B'}} \
+// expected-note@#tc-U2 {{'U2' defined here}}
+}
|
cor3ntin
reviewed
Jun 2, 2025
clang/lib/Sema/SemaTypeTraits.cpp
Outdated
Comment on lines
2148
to
2152
| CXXDestructorDecl *Dtr = D->getDestructor(); | ||
| if (Dtr && Dtr->isUserProvided() && !Dtr->isDefaulted()) | ||
| SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason) | ||
| << diag::TraitNotSatisfiedReason::DeletedDtr << 1 | ||
| << Dtr->getSourceRange(); |
There was a problem hiding this comment.
It might be better to check that the destructor is trivial (Dtr->isTrivial())
There was a problem hiding this comment.
Fixed, now using Dtr->isTrivial()
clang/lib/Sema/SemaTypeTraits.cpp
Outdated
Comment on lines
2115
to
2130
| if (D->isUnion()) { | ||
| auto DiagSPM = [&](CXXSpecialMemberKind K, bool Has) { | ||
| if (Has) | ||
| SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason) | ||
| << diag::TraitNotSatisfiedReason::UnionWithUserDeclaredSMF << K; | ||
| }; | ||
| DiagSPM(CXXSpecialMemberKind::CopyConstructor, | ||
| D->hasUserDeclaredCopyConstructor()); | ||
| DiagSPM(CXXSpecialMemberKind::CopyAssignment, | ||
| D->hasUserDeclaredCopyAssignment()); | ||
| DiagSPM(CXXSpecialMemberKind::MoveConstructor, | ||
| D->hasUserDeclaredMoveConstructor()); | ||
| DiagSPM(CXXSpecialMemberKind::MoveAssignment, | ||
| D->hasUserDeclaredMoveAssignment()); | ||
| return; | ||
| } |
There was a problem hiding this comment.
There is no special rules for unions for trivially copyable
clang/lib/Sema/SemaTypeTraits.cpp
Outdated
Comment on lines
2132
to
2147
| if (!D->hasSimpleMoveConstructor() && !D->hasSimpleCopyConstructor()) { | ||
| const auto *Decl = cast<CXXConstructorDecl>( | ||
| LookupSpecialMemberFromXValue(SemaRef, D, /*Assign=*/false)); | ||
| if (Decl && Decl->isUserProvided()) | ||
| SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason) | ||
| << diag::TraitNotSatisfiedReason::UserProvidedCtr | ||
| << Decl->isMoveConstructor() << Decl->getSourceRange(); | ||
| } | ||
| if (!D->hasSimpleMoveAssignment() && !D->hasSimpleCopyAssignment()) { | ||
| CXXMethodDecl *Decl = | ||
| LookupSpecialMemberFromXValue(SemaRef, D, /*Assign=*/true); | ||
| if (Decl && Decl->isUserProvided()) | ||
| SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason) | ||
| << diag::TraitNotSatisfiedReason::UserProvidedAssign | ||
| << Decl->isMoveAssignmentOperator() << Decl->getSourceRange(); | ||
| } |
There was a problem hiding this comment.
For trivially copyable, the rule is that all special members that exist must be trivial.
https://eel.is/c++draft/class.prop#1
I think it would be better to iterate over all eligible special member functions, and emit a diagnostic for any one that is not trivial (using D->methods(), FunctionDecl::isIneligibleOrNotSelected, FunctionDecl::getDefaultedFunctionKind, and FunctionDecl::isTrivial
There was a problem hiding this comment.
Fixed it, now I'm iterating over all special member functions
clang/lib/Sema/SemaTypeTraits.cpp
Outdated
Comment on lines
2166
to
2168
| if (T.hasNonTrivialObjCLifetime()) | ||
| SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason) | ||
| << diag::TraitNotSatisfiedReason::HasArcLifetime; |
There was a problem hiding this comment.
Arc lifetimes do not impact the ability to copy a type afaik (but they might impact the ability to copy a class containing them, in which case they should have a deleted constructor) @ojhunt
erichkeane
approved these changes
Jun 2, 2025
| @@ -1776,6 +1777,8 @@ def note_unsatisfied_trait_reason | |||
| "%VBase{has a virtual base %1}|" | |||
| "%NRBase{has a non-trivially-relocatable base %1}|" | |||
| "%NRField{has a non-trivially-relocatable member %1 of type %2}|" | |||
| "%NTCBase{has a non-trivially-copyable base %1}|" | |||
There was a problem hiding this comment.
The enum names here are inconsistent... non-trivially-relocatable took only NR whereas this is taking NTC. That said, I VASTLY prefer the T be present. Not in the scope here of course, but a followup tochange those NR to NTR perhaps is valuable.
There was a problem hiding this comment.
Changed NR to NTR
| @@ -1922,6 +1924,7 @@ static std::optional<TypeTrait> StdNameToTypeTrait(StringRef Name) { | |||
| return llvm::StringSwitch<std::optional<TypeTrait>>(Name) | |||
| .Case("is_trivially_relocatable", | |||
| TypeTrait::UTT_IsCppTriviallyRelocatable) | |||
| .Case("is_trivially_copyable", TypeTrait::UTT_IsTriviallyCopyable) | |||
There was a problem hiding this comment.
2 isnt the number, but at a certain number of entries here, we're going to want to tablegen/.def file this. Perhaps in TokenKinds.def?
There was a problem hiding this comment.
I'm not sure if I did it right, but I couldn't figure out how from TYPE_TRAIT_1 could get a name without the "__" prefix. So I made a TYPE_TRAIT_DIAG for this
There was a problem hiding this comment.
Ah, no, I was suggesting adding a new field to the TYPE_TRAIT_1/TYPE_TRAIT_N/etc variants. It does end up being a little bit of extra work, which is why I was mentioning it more as a followup/future direction here instead.
There was a problem hiding this comment.
Should I revert changes?
There was a problem hiding this comment.
Yes, I'd suggest going back to the original, unless you want to do all the work for updating the TYPE_TRAIT_N/TYPE_TRAIT_1 (which I'm not requiring here at all!). Just something to think about for a future direction.
There was a problem hiding this comment.
@erichkeane yeah, maybe we should move the whole type traits thing to a tablegen file at some point... but it seems involved
shafik
reviewed
Jun 2, 2025
| // expected-error@-1 {{static assertion failed due to requirement '__is_trivially_copyable(trivially_copyable::S)'}} \ | ||
| // expected-note@-1 {{'S' is not trivially copyable}} \ | ||
| // expected-note@-1 {{because it has a virtual base 'B'}} \ | ||
| // expected-note@-1 {{because it has a non-trivially-copyable base 'B'}} \ |
There was a problem hiding this comment.
We should test a case in which there are multiple non-trvial-copyable bases.
There was a problem hiding this comment.
Added some tests for this case
There was a problem hiding this comment.
I see the other test case you added but I don't see the test case for multiple base classes. I am guessing maybe you missed a commit.
There was a problem hiding this comment.
It seems that I have added a tests with multiple base classes (structs S5, S6). Or do you need a test with three or more? Or what do you mean by "multiple base classes"?
There was a problem hiding this comment.
It seems that I've figured it out. I added one more test.
shafik
reviewed
Jun 2, 2025
| auto SpecialMemberKind = | ||
| SemaRef.getDefaultedFunctionKind(Method).asSpecialMember(); | ||
| switch (SpecialMemberKind) { | ||
| case CXXSpecialMemberKind::CopyConstructor: |
There was a problem hiding this comment.
If every single case label covered in the tests?
There was a problem hiding this comment.
Added tests for every single case
clang/lib/Sema/SemaTypeTraits.cpp
Outdated
| << D->getDestructor()->getSourceRange(); | ||
|
|
||
| for (const CXXMethodDecl *Method : D->methods()) { | ||
| if (Method->isIneligibleOrNotSelected() || Method->isTrivial() || |
There was a problem hiding this comment.
Are all three conditions in the if statement covered by the test?
There was a problem hiding this comment.
Added coverage for two conditions, removed Method->isIneligibleOrNotSelected(). Frankly speaking, I could not create a case when Method->isIneligibleOrNotSelected() is true and Method->isTrivial() || !Method->isUserProvided() is not
| SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason) | ||
| << diag::TraitNotSatisfiedReason::VBase << B.getType() | ||
| << B.getSourceRange(); | ||
| if (!B.getType().isTriviallyCopyableType(D->getASTContext())) { |
There was a problem hiding this comment.
Is this if statement covered in the tests below?
There was a problem hiding this comment.
Added test for it
egorshamshura
force-pushed
the
explain-trivial-copyable
branch
from
a9211af to
2401565
Compare
cor3ntin
reviewed
Jun 3, 2025
| @@ -1922,6 +1924,7 @@ static std::optional<TypeTrait> StdNameToTypeTrait(StringRef Name) { | |||
| return llvm::StringSwitch<std::optional<TypeTrait>>(Name) | |||
| .Case("is_trivially_relocatable", | |||
| TypeTrait::UTT_IsCppTriviallyRelocatable) | |||
| .Case("is_trivially_copyable", TypeTrait::UTT_IsTriviallyCopyable) | |||
There was a problem hiding this comment.
@erichkeane yeah, maybe we should move the whole type traits thing to a tablegen file at some point... but it seems involved
clang/lib/Sema/SemaTypeTraits.cpp
Outdated
Comment on lines
2139
to
2142
| default: { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggested change
| default: { | |
| break; | |
| } | |
| } | |
| default: | |
| break; | |
| } |
clang/lib/Sema/SemaTypeTraits.cpp
Outdated
Comment on lines
2144
to
2149
| CXXDestructorDecl *Dtr = D->getDestructor(); | ||
| if (Dtr && !Dtr->isTrivial()) | ||
| SemaRef.Diag(Loc, diag::note_unsatisfied_trait_reason) | ||
| << diag::TraitNotSatisfiedReason::DeletedDtr << 1 | ||
| << Dtr->getSourceRange(); | ||
| } |
There was a problem hiding this comment.
Can you move that to the previous place where we talked about the destructor?
We should say either "has a deleted destructor" or "has a non-trivial destructor" (but not both)
There was a problem hiding this comment.
Oh, fixed it, decided to write "has a deleted destructor". Is it good now?
cor3ntin
approved these changes
Jun 6, 2025
Yep, that would be great It seems that I accidentally updated the branch, I hope it's not critical. |
|
@egorshamshura Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/17541 Here is the relevant piece of the build log for the reference |
rorth
pushed a commit
to rorth/llvm-project
that referenced
this pull request
Jun 11, 2025
DhruvSrivastavaX
pushed a commit
to DhruvSrivastavaX/lldb-for-aix
that referenced
this pull request
Jun 12, 2025
tomtor
pushed a commit
to tomtor/llvm-project
that referenced
this pull request
Jun 14, 2025
egorshamshura
changed the title
is trivial copyable evaluated to false.
egorshamshura
changed the title
is trivial copyable evaluated to false.is_trivial_copyable evaluated to false.
egorshamshura
changed the title
is_trivial_copyable evaluated to false.is_trivially_copyable evaluated to false.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.