llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Bhuminjay Soni (11happy) <details> <summary>Changes</summary> **Overview:** This pull request fixes #<!-- -->47355 where in the Clang compiler's range-loop-analysis incorrectly checks for trivial copyability instead of trivial copy constructibility, leading to erroneous warnings. **Changes Made:** - The changes made in this commit address the issue by introducing a new member function `isTriviallyCopyConstructible` in the `CXXRecordDecl` class and implementing it in the associated files `(clang/include/clang/AST/DeclCXX.h, clang/lib/AST/DeclCXX.cpp)`. Additionally, modifications are made in `clang/include/clang/AST/Type.h` and `clang/lib/AST/Type.cpp` to support the new function. The implementation checks for trivial copy constructibility, distinguishing it from the existing `isTriviallyCopyable` check. The issue in `clang/lib/Sema/SemaStmt.cpp` is also addressed by updating the conditional check in the `DiagnoseForRangeConstVariableCopies` function to use the new `isTriviallyCopyConstructibleType` function. Overall, these changes aim to correct the range-loop-analysis by ensuring it checks for trivial copy constructibility rather than trivial copyability. **Testing:** - Tested the updated code. - Verified that other functionalities remain unaffected.  **Dependencies:** - No dependencies on other pull requests. **References:** - https://cplusplus.com/reference/type_traits/is_trivially_copy_constructible/ - https://en.cppreference.com/w/cpp/named_req/CopyConstructible - https://cplusplus.com/reference/type_traits/is_trivially_copyable/ **CC:** - @<!-- -->Endilll , @<!-- -->r4nt , @<!-- -->AaronBallman --- Full diff: https://github.com/llvm/llvm-project/pull/76680.diff 5 Files Affected: - (modified) clang/include/clang/AST/DeclCXX.h (+3) - (modified) clang/include/clang/AST/Type.h (+3) - (modified) clang/lib/AST/DeclCXX.cpp (+13) - (modified) clang/lib/AST/Type.cpp (+43) - (modified) clang/lib/Sema/SemaStmt.cpp (+1-1) ``````````diff diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h index 432293583576b5..3ac0d6a9e083cd 100644 --- a/clang/include/clang/AST/DeclCXX.h +++ b/clang/include/clang/AST/DeclCXX.h @@ -1425,6 +1425,9 @@ class CXXRecordDecl : public RecordDecl { /// (C++11 [class]p6). bool isTriviallyCopyable() const; + /// Determine whether this class is considered trivially copyable per + bool isTriviallyCopyConstructible() const; + /// Determine whether this class is considered trivial. /// /// C++11 [class]p6: diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h index 1afa693672860f..bce2256f96a828 100644 --- a/clang/include/clang/AST/Type.h +++ b/clang/include/clang/AST/Type.h @@ -917,6 +917,9 @@ class QualType { /// Return true if this is a trivially copyable type (C++0x [basic.types]p9) bool isTriviallyCopyableType(const ASTContext &Context) const; + /// Return true if this is a trivially copyable type + bool isTriviallyCopyConstructibleType(const ASTContext &Context) const; + /// Return true if this is a trivially relocatable type. bool isTriviallyRelocatableType(const ASTContext &Context) const; diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp index c944862fcefeee..98b0a6dc28ea2f 100644 --- a/clang/lib/AST/DeclCXX.cpp +++ b/clang/lib/AST/DeclCXX.cpp @@ -587,6 +587,19 @@ bool CXXRecordDecl::isTriviallyCopyable() const { return true; } +bool CXXRecordDecl::isTriviallyCopyConstructible() const { + + // A trivially copy constructible class is a class that: + // -- has no non-trivial copy constructors, + if (hasNonTrivialCopyConstructor()) + return false; + // -- has a trivial destructor. + if (!hasTrivialDestructor()) + return false; + + return true; +} + void CXXRecordDecl::markedVirtualFunctionPure() { // C++ [class.abstract]p2: // A class is abstract if it has at least one pure virtual function. diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp index 160a725939ccd4..9c8b25798a0a95 100644 --- a/clang/lib/AST/Type.cpp +++ b/clang/lib/AST/Type.cpp @@ -2644,6 +2644,49 @@ bool QualType::isTriviallyCopyableType(const ASTContext &Context) const { return false; } +bool QualType::isTriviallyCopyConstructibleType( + const ASTContext &Context) const { + if ((*this)->isArrayType()) + return Context.getBaseElementType(*this).isTriviallyCopyConstructibleType( + Context); + + if (hasNonTrivialObjCLifetime()) + return false; + + // C++11 [basic.types]p9 - See Core 2094 + // Scalar types, trivially copyable class types, arrays of such types, and + // cv-qualified versions of these types are collectively + // called trivially copy constructible types. + + QualType CanonicalType = getCanonicalType(); + if (CanonicalType->isDependentType()) + return false; + + if (CanonicalType->isSizelessBuiltinType()) + return true; + + // Return false for incomplete types after skipping any incomplete array types + // which are expressly allowed by the standard and thus our API. + if (CanonicalType->isIncompleteType()) + return false; + + // As an extension, Clang treats vector types as Scalar types. + if (CanonicalType->isScalarType() || CanonicalType->isVectorType()) + return true; + + if (const auto *RT = CanonicalType->getAs<RecordType>()) { + if (const auto *ClassDecl = dyn_cast<CXXRecordDecl>(RT->getDecl())) { + if (!ClassDecl->isTriviallyCopyConstructible()) + return false; + } + + return true; + } + + // No other types can match. + return false; +} + bool QualType::isTriviallyRelocatableType(const ASTContext &Context) const { QualType BaseElementType = Context.getBaseElementType(*this); diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp index f0b03db690843a..21efe25ed84a3d 100644 --- a/clang/lib/Sema/SemaStmt.cpp +++ b/clang/lib/Sema/SemaStmt.cpp @@ -3200,7 +3200,7 @@ static void DiagnoseForRangeConstVariableCopies(Sema &SemaRef, // (The function `getTypeSize` returns the size in bits.) ASTContext &Ctx = SemaRef.Context; if (Ctx.getTypeSize(VariableType) <= 64 * 8 && - (VariableType.isTriviallyCopyableType(Ctx) || + (VariableType.isTriviallyCopyConstructibleType(Ctx) || hasTrivialABIAttr(VariableType))) return; `````````` </details> https://github.com/llvm/llvm-project/pull/76680 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
