Index: clang-tidy/misc/CMakeLists.txt =================================================================== --- clang-tidy/misc/CMakeLists.txt +++ clang-tidy/misc/CMakeLists.txt @@ -14,6 +14,7 @@ NoexceptMoveConstructorCheck.cpp StaticAssertCheck.cpp SwappedArgumentsCheck.cpp + ThrowByValueCatchByReferenceCheck.cpp UndelegatedConstructor.cpp UnusedAliasDeclsCheck.cpp UnusedParametersCheck.cpp Index: clang-tidy/misc/MiscTidyModule.cpp =================================================================== --- clang-tidy/misc/MiscTidyModule.cpp +++ clang-tidy/misc/MiscTidyModule.cpp @@ -22,6 +22,7 @@ #include "NoexceptMoveConstructorCheck.h" #include "StaticAssertCheck.h" #include "SwappedArgumentsCheck.h" +#include "ThrowByValueCatchByReferenceCheck.h" #include "UndelegatedConstructor.h" #include "UniqueptrResetReleaseCheck.h" #include "UnusedAliasDeclsCheck.h" @@ -58,6 +59,8 @@ "misc-static-assert"); CheckFactories.registerCheck( "misc-swapped-arguments"); + CheckFactories.registerCheck( + "misc-throw-by-value-catch-by-reference"); CheckFactories.registerCheck( "misc-undelegated-constructor"); CheckFactories.registerCheck( Index: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h =================================================================== --- /dev/null +++ clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h @@ -0,0 +1,43 @@ +//===--- ThrowByValueCatchByReferenceCheck.h - clang-tidy--------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { + +///\brief checks for locations that do not throw by value +// or catch by reference. +// The check is C++ only. It checks that all throw locations +// throw by value and not by pointer. Additionally it +// contains an option ("CheckThrowTemporaries", default value "true") that +// checks that thrown objects are anonymous temporaries. It is also +// acceptable for this check to throw string literals. +// This test checks that exceptions are caught by reference +// and not by value or pointer. It will not warn when catching +// pointer to char, wchar_t, char16_t or char32_t. This is +// due to not warning on throwing string literals. +class ThrowByValueCatchByReferenceCheck : public ClangTidyCheck { +public: + ThrowByValueCatchByReferenceCheck(StringRef Name, ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + bool checkAnonymousTemporaries; +}; + +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H Index: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp @@ -0,0 +1,137 @@ +//===--- ThrowByValueCatchByReferenceCheck.cpp - clang-tidy----------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "ThrowByValueCatchByReferenceCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/AST/OperationKinds.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { + +ThrowByValueCatchByReferenceCheck::ThrowByValueCatchByReferenceCheck( + StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + checkAnonymousTemporaries(Options.get("CheckThrowTemporaries", "true") == + "true") {} + +void ThrowByValueCatchByReferenceCheck::registerMatchers(MatchFinder *Finder) { + // Only register the matchers for C++ + if (!getLangOpts().CPlusPlus) + return; + + Finder->addMatcher(throwExpr().bind("throw"), this); + Finder->addMatcher(catchStmt().bind("catch"), this); +} + +void ThrowByValueCatchByReferenceCheck::storeOptions( + ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "CheckThrowTemporaries", "true"); +} + +void ThrowByValueCatchByReferenceCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *throwExpr = Result.Nodes.getNodeAs("throw"); + if (throwExpr != nullptr) { + auto *subExpr = throwExpr->getSubExpr(); + auto qualType = subExpr->getType(); + auto *type = qualType.getTypePtr(); + if (type->isPointerType()) { // throwing a pointer + // check for strng literal - which is safe + // e.g. throw "simple exception" + if (isa(subExpr)) { + auto *inner = cast(subExpr)->getSubExpr(); + if (isa(inner)) + return; + } + diag(subExpr->getLocStart(), "avoid throwing pointer"); + } + // if not thrown by pointer, then it has been thrown + // by value, which is OK. + // additional check if thrown value is a RValue + // according to guideline: + // https://www.securecoding.cert.org/confluence/display/cplusplus/ERR09-CPP.+Throw+anonymous+temporaries + + // algorithm + // from CXXThrowExpr, move through all casts until you either encounter an + // LValueToRValue-Cast or a + // CXXConstructExpr. + // if it's a LValueToRValue-Cast, emit message + // if it's a CopyOrMoveConstructor, emit message if after casts, it + // contains a DeclRefExpr for the parameter. + // otherwise - do not emit a message. + if (checkAnonymousTemporaries) { + bool emit = false; + auto *currentSubExpr = subExpr; + while (isa(currentSubExpr)) { + auto *currentCast = cast(currentSubExpr); + if (currentCast->getCastKind() == clang::CK_LValueToRValue) { + emit = true; + break; + } + } + if (!emit) { + // Check for copy construction + // we're now through all potential casts. This should be a + // CXXConstructExpr in most cases + const CXXConstructExpr *constructorCall; + if (isa(currentSubExpr)) + constructorCall = cast(currentSubExpr); + else + constructorCall = nullptr; + // if it's a copy constructor it could copy from a lvalue + if (constructorCall && + constructorCall->getConstructor()->isCopyOrMoveConstructor()) { + // again skip all potential casts + auto argIter = + constructorCall + ->arg_begin(); // there's only one for copy constructors + auto *currentSubExpr = *argIter; + while (isa(currentSubExpr)) { + auto *currentCast = cast(currentSubExpr); + currentSubExpr = currentCast->getSubExpr(); + } + // if the subexpr is now a DeclRefExpr, it's a real variable + if (isa(currentSubExpr)) + emit = true; + } + } + if (emit) + diag(subExpr->getLocStart(), "prefer throwing anonymous temporaries"); + } + } + const auto *catchStmt = Result.Nodes.getNodeAs("catch"); + if (catchStmt != nullptr) { + auto caughtType = catchStmt->getCaughtType(); + if (caughtType.isNull()) + return; + auto *type = caughtType.getTypePtr(); + auto *varDecl = catchStmt->getExceptionDecl(); + if (type->isPointerType()) { + auto canonical = type->getCanonicalTypeInternal().getTypePtr(); + // check if pointed-to-type is char, wchar_t, char16_t, char32_t + auto *pointeeType = + cast(canonical)->getPointeeType().getTypePtr(); + if (pointeeType->isAnyCharacterType() == false) { + diag(varDecl->getLocStart(), "catch by reference"); + } + } else if (!type->isReferenceType()) { + // not pointer, not reference this means it must be "by value" + // do not advice on built-in types - catching them by value + // is ok + if (!caughtType.isTrivialType(*Result.Context)) + diag(varDecl->getLocStart(), "catch by reference"); + } + } +} + +} // namespace tidy +} // namespace clang Index: test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp =================================================================== --- /dev/null +++ test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp @@ -0,0 +1,138 @@ +// RUN: %python %S/check_clang_tidy.py %s misc-throw-by-value-catch-by-reference %t + +//#include + +class logic_error { +public: + logic_error(const char *message) {} +}; + +typedef logic_error *logic_ptr; +typedef logic_ptr logic_double_typedef; + +int lastException; + +logic_error CreateException() { return logic_error("created"); } + +void testThrowFunc() { + throw new logic_error("by pointer"); + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: avoid throwing pointer + // [misc-throw-by-value-catch-by-reference] + logic_ptr tmp = new logic_error("by pointer"); + throw tmp; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: avoid throwing pointer + // [misc-throw-by-value-catch-by-reference] + // CHECK-MESSAGES: :[[@LINE-3]]:9: warning: prefer throwing anonymous + // temporaries [misc-throw-by-value-catch-by-reference] + throw logic_error("by value"); + auto *literal = "test"; + throw logic_error(literal); + throw "test string literal"; + throw L"throw wide string literal"; + const char *characters = 0; + throw characters; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: avoid throwing pointer + // [misc-throw-by-value-catch-by-reference] + // CHECK-MESSAGES: :[[@LINE-3]]:9: warning: prefer throwing anonymous + // temporaries [misc-throw-by-value-catch-by-reference] + logic_error lvalue("lvalue"); + throw lvalue; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: prefer throwing anonymous + // temporaries [misc-throw-by-value-catch-by-reference] + + // can be enabled once std::move can be included + // throw std::move(lvalue) + int &ex = lastException; + throw ex; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: prefer throwing anonymous + // temporaries [misc-throw-by-value-catch-by-reference] + throw CreateException(); +} + +void throwReferenceFunc(logic_error &ref) { + throw ref; + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: prefer throwing anonymous + // temporaries [misc-throw-by-value-catch-by-reference] +} + +void catchByPointer() { + try { + testThrowFunc(); + } catch (logic_error *e) { + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch by reference + // [misc-throw-by-value-catch-by-reference] + } +} + +void catchByValue() { + try { + testThrowFunc(); + } catch (logic_error e) { + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch by reference + // [misc-throw-by-value-catch-by-reference] + } +} + +void catchByReference() { + try { + testThrowFunc(); + } catch (logic_error &e) { + } +} + +void catchByConstReference() { + try { + testThrowFunc(); + } catch (const logic_error &e) { + } +} + +void catchTypedef() { + try { + testThrowFunc(); + } catch (logic_ptr) { + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch by reference + // [misc-throw-by-value-catch-by-reference] + } +} + +void catchAll() { + try { + testThrowFunc(); + } catch (...) { + } +} + +void catchLiteral() { + try { + testThrowFunc(); + } catch (const char *) { + } catch (const wchar_t *) { + // disabled for now until it is clear + // how to enable them in the test + //} catch (const char16_t*) { + //} catch (const char32_t*) { + } +} + +// catching fundamentals should not warn +void catchFundamental() { + try { + testThrowFunc(); + } catch (int) { + } catch (double) { + } catch (unsigned long) { + } +} + +struct TrivialType { + double x; + double y; +}; + +void catchTrivial() { + try { + testThrowFunc(); + } catch (TrivialType) { + } +}