AMS21 updated this revision to Diff 509013. AMS21 added a comment. Implemented suggested fixes from reviews
Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146922/new/ https://reviews.llvm.org/D146922 Files: clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor-fix.cpp clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor.cpp =================================================================== --- clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor.cpp +++ clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor.cpp @@ -4,7 +4,7 @@ A(A &&); // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: move constructors should be marked noexcept [performance-noexcept-move-constructor] A &operator=(A &&); - // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: move assignment operators should + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: move assignment operators should be marked noexcept [performance-noexcept-move-constructor] }; struct B { @@ -13,6 +13,95 @@ // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor] }; +template <typename> +struct C +{ + C(C &&); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: move constructors should be marked noexcept [performance-noexcept-move-constructor] + C& operator=(C &&); + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: move assignment operators should be marked noexcept [performance-noexcept-move-constructor] +}; + +struct D +{ + static constexpr bool kFalse = false; + D(D &&) noexcept(kFalse) = default; + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor] + D& operator=(D &&) noexcept(kFalse) = default; + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: noexcept specifier on the move assignment operator evaluates to 'false' +}; + +template <typename> +struct E +{ + static constexpr bool kFalse = false; + E(E &&) noexcept(kFalse); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor] + E& operator=(E &&) noexcept(kFalse); + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: noexcept specifier on the move assignment operator evaluates to 'false' +}; + +template <typename> +struct F +{ + static constexpr bool kFalse = false; + F(F &&) noexcept(kFalse) = default; + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor] + F& operator=(F &&) noexcept(kFalse) = default; + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: noexcept specifier on the move assignment operator evaluates to 'false' +}; + +struct Field +{ + Field() = default; + Field(Field&&) noexcept(false) { + } + Field& operator=(Field &&) noexcept(false) { + return *this; + } +}; + +struct G { + G(G &&) = default; + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: move constructors should be marked noexcept [performance-noexcept-move-constructor] + G& operator=(G &&) = default; + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: move assignment operators should be marked noexcept [performance-noexcept-move-constructor] + + Field field; +}; + +void throwing_function() noexcept(false) {} + +struct H { + H(H &&) noexcept(noexcept(throwing_function())); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor] + H &operator=(H &&) noexcept(noexcept(throwing_function())); + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: noexcept specifier on the move assignment operator evaluates to 'false' +}; + +template <typename> +struct I { + I(I &&) noexcept(noexcept(throwing_function())); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor] + I &operator=(I &&) noexcept(noexcept(throwing_function())); + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: noexcept specifier on the move assignment operator evaluates to 'false' +}; + +template <typename TemplateType> struct TemplatedType { + static void f() {} +}; + +template <> struct TemplatedType<int> { + static void f() noexcept {} +}; + +struct J { + J(J &&) noexcept(noexcept(TemplatedType<double>::f())); + // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: noexcept specifier on the move constructor evaluates to 'false' [performance-noexcept-move-constructor] + J &operator=(J &&) noexcept(noexcept(TemplatedType<double>::f())); + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: noexcept specifier on the move assignment operator evaluates to 'false' [performance-noexcept-move-constructor] +}; + class OK {}; void f() { @@ -52,3 +141,86 @@ OK5(OK5 &&) noexcept(true) = default; OK5 &operator=(OK5 &&) noexcept(true) = default; }; + +struct OK6 { + OK6(OK6 &&) = default; + OK6& operator=(OK6 &&) = default; +}; + +template <typename> +struct OK7 { + OK7(OK7 &&) = default; + OK7& operator=(OK7 &&) = default; +}; + +template <typename> +struct OK8 { + OK8(OK8 &&) noexcept = default; + OK8& operator=(OK8 &&) noexcept = default; +}; + +template <typename> +struct OK9 { + OK9(OK9 &&) noexcept(true) = default; + OK9& operator=(OK9 &&) noexcept(true) = default; +}; + +template <typename> +struct OK10 { + OK10(OK10 &&) noexcept(false) = default; + OK10& operator=(OK10 &&) noexcept(false) = default; +}; + +template <typename> +struct OK11 { + OK11(OK11 &&) = delete; + OK11& operator=(OK11 &&) = delete; +}; + +void nothrowing_function() noexcept {} + +struct OK12 { + OK12(OK12 &&) noexcept(noexcept(nothrowing_function())); + OK12 &operator=(OK12 &&) noexcept(noexcept(nothrowing_function)); +}; + +struct OK13 { + OK13(OK13 &&) noexcept(noexcept(nothrowing_function)) = default; + OK13 &operator=(OK13 &&) noexcept(noexcept(nothrowing_function)) = default; +}; + +template <typename> struct OK14 { + OK14(OK14 &&) noexcept(noexcept(TemplatedType<int>::f())); + OK14 &operator=(OK14 &&) noexcept(noexcept(TemplatedType<int>::f())); +}; + +struct OK15 { + OK15(OK15 &&) = default; + OK15 &operator=(OK15 &&) = default; + + int member; +}; + +template <typename> +struct OK16 { + OK16(OK16 &&) = default; + OK16 &operator=(OK16 &&) = default; + + int member; +}; + +struct OK17 +{ + OK17(OK17 &&) = default; + OK17 &operator=(OK17 &&) = default; + OK empty_field; +}; + +template <typename> +struct OK18 +{ + OK18(OK18 &&) = default; + OK18 &operator=(OK18 &&) = default; + + OK empty_field; +}; Index: clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor-fix.cpp =================================================================== --- clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor-fix.cpp +++ clang-tools-extra/test/clang-tidy/checkers/performance/noexcept-move-constructor-fix.cpp @@ -31,9 +31,7 @@ }; C_3::C_3(C_3&& a) = default; -// CHECK-FIXES: ){{.*}}noexcept{{.*}} = default; C_3& C_3::operator=(C_3&& a) = default; -// CHECK-FIXES: ){{.*}}noexcept{{.*}} = default; template <class T> struct C_4 { @@ -41,7 +39,6 @@ // CHECK-FIXES: ){{.*}}noexcept{{.*}} {} ~C_4() {} C_4& operator=(C_4&& a) = default; -// CHECK-FIXES: ){{.*}}noexcept{{.*}} = default; }; template <class T> @@ -50,7 +47,6 @@ // CHECK-FIXES:){{.*}}noexcept{{.*}} {} ~C_5() {} auto operator=(C_5&& a)->C_5<T> = default; -// CHECK-FIXES:){{.*}}noexcept{{.*}} = default; }; template <class T> @@ -64,4 +60,4 @@ template <class T> auto C_6<T>::operator=(C_6<T>&& a) -> C_6<T> {} -// CHECK-FIXES: ){{.*}}noexcept{{.*}} {} \ No newline at end of file +// CHECK-FIXES: ){{.*}}noexcept{{.*}} {} Index: clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp =================================================================== --- clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp +++ clang-tools-extra/clang-tidy/performance/NoexceptMoveConstructorCheck.cpp @@ -16,6 +16,116 @@ namespace clang::tidy::performance { +namespace { + +enum CanMoveConstrcutorThrowResult { + CMCT_CanThrow, ///< We are sure the move constructor can throw + CMCT_CannotThrow, ///< We are sure the move constrcutor cannot throw + CMCT_Unknown, ///< We don't know if the move constrcutor can throw or not +}; + +CanMoveConstrcutorThrowResult evaluateFunctionEST(const FunctionDecl *Func) { + const auto *FunProto = Func->getType()->getAs<FunctionProtoType>(); + if (!FunProto) + return CMCT_Unknown; + + switch (FunProto->canThrow()) { + case CT_Cannot: + return CMCT_CannotThrow; + case CT_Dependent: { + const Expr *NoexceptExpr = FunProto->getNoexceptExpr(); + bool Result; + return (NoexceptExpr && !NoexceptExpr->isValueDependent() && + NoexceptExpr->EvaluateAsBooleanCondition( + Result, Func->getASTContext(), true) && + Result) + ? CMCT_CannotThrow + : CMCT_CanThrow; + } + default: + return CMCT_CanThrow; + }; +} + +CanMoveConstrcutorThrowResult +canMoveConstructorThrow(const FunctionDecl *FuncDecl); + +CanMoveConstrcutorThrowResult +canFieldMoveConstructorThrow(const FieldDecl *FDecl, bool TestConstructor) { + if (!FDecl) + return CMCT_Unknown; + + const ASTContext &Context = FDecl->getASTContext(); + + if (auto *FieldRecordType = + Context.getBaseElementType(FDecl->getType())->getAs<RecordType>()) { + if (const auto *FieldRecordDecl = + cast<CXXRecordDecl>(FieldRecordType->getDecl())) { + + // Trivial implies noexcept + if ((FieldRecordDecl->hasTrivialMoveConstructor() && TestConstructor) || + (FieldRecordDecl->hasTrivialMoveAssignment() && !TestConstructor)) + return CMCT_CannotThrow; + + for (const auto *Method : FieldRecordDecl->methods()) { + const auto *Ctor = dyn_cast<CXXConstructorDecl>(Method); + + if ((TestConstructor && Ctor && Ctor->isMoveConstructor()) || + (Method->isMoveAssignmentOperator() && !TestConstructor)) + return canMoveConstructorThrow(Method); + } + } + } + + // We assume non record types cannot throw + return CMCT_CannotThrow; +} + +CanMoveConstrcutorThrowResult +resolveUnevaluatedOrDefaulted(const FunctionDecl *FuncDecl) { + const auto *FunProto = FuncDecl->getType()->getAs<FunctionProtoType>(); + if (!FunProto) + return CMCT_Unknown; + + const CXXMethodDecl *MethodDecl = cast<CXXMethodDecl>(FuncDecl); + if (!MethodDecl || MethodDecl->isInvalidDecl()) + return CMCT_Unknown; + + const CXXRecordDecl *RecordDecl = MethodDecl->getParent(); + if (!RecordDecl || RecordDecl->isInvalidDecl()) + return CMCT_Unknown; + + const ASTContext &Context = FuncDecl->getASTContext(); + if (Context.getRecordType(RecordDecl).isNull()) + return CMCT_Unknown; + + const bool IsConstructor = !MethodDecl->isMoveAssignmentOperator(); + + // FIXME: We currectly ignore virtual and non virtual bases + + for (const auto *FDecl : RecordDecl->fields()) + if (!FDecl->isInvalidDecl() && !FDecl->isUnnamedBitfield()) + if (canFieldMoveConstructorThrow(FDecl, IsConstructor) == CMCT_CanThrow) + return CMCT_CanThrow; + + return CMCT_CannotThrow; +} + +CanMoveConstrcutorThrowResult +canMoveConstructorThrow(const FunctionDecl *FuncDecl) { + const auto *FunProto = FuncDecl->getType()->getAs<FunctionProtoType>(); + if (!FunProto) + return CMCT_Unknown; + + const ExceptionSpecificationType EST = FunProto->getExceptionSpecType(); + + if (EST == EST_Unevaluated || (EST == EST_None && FuncDecl->isDefaulted())) + return resolveUnevaluatedOrDefaulted(FuncDecl); + + return evaluateFunctionEST(FuncDecl); +} +} // namespace + void NoexceptMoveConstructorCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( cxxMethodDecl(anyOf(cxxConstructorDecl(), hasOverloadedOperatorName("=")), @@ -36,42 +146,41 @@ return; } - const auto *ProtoType = Decl->getType()->castAs<FunctionProtoType>(); + const CanMoveConstrcutorThrowResult result = canMoveConstructorThrow(Decl); - if (isUnresolvedExceptionSpec(ProtoType->getExceptionSpecType())) + if (result != CMCT_CanThrow) return; - if (!isNoexceptExceptionSpec(ProtoType->getExceptionSpecType())) { - auto Diag = diag(Decl->getLocation(), - "move %select{assignment operator|constructor}0s should " - "be marked noexcept") - << IsConstructor; - // Add FixIt hints. - SourceManager &SM = *Result.SourceManager; - assert(Decl->getNumParams() > 0); - SourceLocation NoexceptLoc = Decl->getParamDecl(Decl->getNumParams() - 1) - ->getSourceRange() - .getEnd(); - if (NoexceptLoc.isValid()) - NoexceptLoc = Lexer::findLocationAfterToken( - NoexceptLoc, tok::r_paren, SM, Result.Context->getLangOpts(), true); - if (NoexceptLoc.isValid()) - Diag << FixItHint::CreateInsertion(NoexceptLoc, " noexcept "); - return; - } - // Don't complain about nothrow(false), but complain on nothrow(expr) // where expr evaluates to false. - if (ProtoType->canThrow() == CT_Can) { - Expr *E = ProtoType->getNoexceptExpr(); - E = E->IgnoreImplicit(); - if (!isa<CXXBoolLiteralExpr>(E)) { - diag(E->getExprLoc(), + const auto *ProtoType = Decl->getType()->castAs<FunctionProtoType>(); + const Expr *NoexceptExpr = ProtoType->getNoexceptExpr(); + if (NoexceptExpr) { + NoexceptExpr = NoexceptExpr->IgnoreImplicit(); + if (!isa<CXXBoolLiteralExpr>(NoexceptExpr)) { + diag(NoexceptExpr->getExprLoc(), "noexcept specifier on the move %select{assignment " "operator|constructor}0 evaluates to 'false'") << IsConstructor; } + return; } + + auto Diag = diag(Decl->getLocation(), + "move %select{assignment operator|constructor}0s should " + "be marked noexcept") + << IsConstructor; + // Add FixIt hints. + SourceManager &SM = *Result.SourceManager; + assert(Decl->getNumParams() > 0); + SourceLocation NoexceptLoc = + Decl->getParamDecl(Decl->getNumParams() - 1)->getSourceRange().getEnd(); + if (NoexceptLoc.isValid()) + NoexceptLoc = Lexer::findLocationAfterToken( + NoexceptLoc, tok::r_paren, SM, Result.Context->getLangOpts(), true); + if (NoexceptLoc.isValid()) + Diag << FixItHint::CreateInsertion(NoexceptLoc, " noexcept "); + return; } }
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits