randomcppprogrammer updated this revision to Diff 36700.
randomcppprogrammer marked 17 inline comments as done.
randomcppprogrammer added a comment.
reworked code to include the changes suggested by Aaron Ballman
main changes
- will not diagnose on throwing catch variables by value/pointer
- will not diagnose on throwing function parameters (if you are using special
handler functions for throwing)
http://reviews.llvm.org/D11328
Files:
clang-tidy/misc/CMakeLists.txt
clang-tidy/misc/MiscTidyModule.cpp
clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp
clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h
test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp
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,149 @@
+// RUN: %python %S/check_clang_tidy.py %s misc-throw-by-value-catch-by-reference %t
+
+
+class logic_error {
+public:
+ logic_error(const char *message) {}
+};
+
+typedef logic_error *logic_ptr;
+typedef logic_ptr logic_double_typedef;
+
+int lastException;
+
+template <class T> struct remove_reference {typedef T type;};
+template <class T> struct remove_reference<T&> {typedef T type;};
+template <class T> struct remove_reference<T&&> {typedef T type;};
+
+template <typename T>
+typename remove_reference<T>::type&& move(T&& arg) {
+ return static_cast<typename remove_reference<T>::type&&>(arg);
+}
+
+logic_error CreateException() { return logic_error("created"); }
+
+void testThrowFunc() {
+ throw new logic_error("by pointer");
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: throw expression throws a pointer; it should throw a non pointer value instead [misc-throw-by-value-catch-by-reference]
+ logic_ptr tmp = new logic_error("by pointer");
+ throw tmp;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: throw expression should throw anonymous temporary values instead [misc-throw-by-value-catch-by-reference]
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: throw expression throws a pointer; it should throw a non pointer value instead [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: throw expression should throw anonymous temporary values instead [misc-throw-by-value-catch-by-reference]
+ // CHECK-MESSAGES: :[[@LINE-2]]:9: warning: throw expression throws a pointer; it should throw a non pointer value instead [misc-throw-by-value-catch-by-reference]
+ logic_error lvalue("lvalue");
+ throw lvalue;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: throw expression should throw anonymous temporary values instead [misc-throw-by-value-catch-by-reference]
+
+ // can be enabled once std::move can be included
+ throw move(lvalue);
+ int &ex = lastException;
+ throw ex;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: throw expression should throw anonymous temporary values instead [misc-throw-by-value-catch-by-reference]
+ throw CreateException();
+}
+
+void throwReferenceFunc(logic_error &ref) {
+ throw ref;
+}
+
+void catchByPointer() {
+ try {
+ testThrowFunc();
+ } catch (logic_error *e) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch handler catches a pointer value; should throw a non-pointer value and catch by reference instead [misc-throw-by-value-catch-by-reference]
+ }
+}
+
+void catchByValue() {
+ try {
+ testThrowFunc();
+ } catch (logic_error e) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch handler catches a pointer value; should throw a non-pointer value and catch by reference instead [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 handler catches a pointer value; should throw a non-pointer value and catch by reference instead [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) {
+ }
+}
+
+typedef logic_error& fine;
+void additionalTests() {
+ try {
+ } catch (int i) { // ok
+ throw i; // ok
+ } catch (fine e) { // ok
+ throw e; // ok
+ } catch (logic_error *e) {
+ // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch handler catches a pointer value; should throw a non-pointer value and catch by reference instead [misc-throw-by-value-catch-by-reference]
+ throw e; // ok, despite throwing a pointer
+ } catch(...) { // ok
+ throw; // ok
+ }
+}
Index: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h
===================================================================
--- /dev/null
+++ clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h
@@ -0,0 +1,49 @@
+//===--- 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:
+ void diagnoseThrowLocations(const CXXThrowExpr *throwExpr);
+ void diagnoseCatchLocations(const CXXCatchStmt *catchStmt,
+ ASTContext &context);
+ bool isFunctionParameter(const DeclRefExpr *declRefExpr);
+ bool isCatchVariable(const DeclRefExpr *declRefExpr);
+ bool isFunctionOrCatchVar(const DeclRefExpr *declRefExpr);
+ const 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,176 @@
+//===--- 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)) {}
+
+void ThrowByValueCatchByReferenceCheck::registerMatchers(MatchFinder *Finder) {
+ // Only register the matchers for C++
+ if (!getLangOpts().CPlusPlus)
+ return;
+
+ Finder->addMatcher(cxxThrowExpr().bind("throw"), this);
+ Finder->addMatcher(cxxCatchStmt().bind("catch"), this);
+}
+
+void ThrowByValueCatchByReferenceCheck::storeOptions(
+ ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "CheckThrowTemporaries", true);
+}
+
+void ThrowByValueCatchByReferenceCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ diagnoseThrowLocations(Result.Nodes.getNodeAs<CXXThrowExpr>("throw"));
+ diagnoseCatchLocations(Result.Nodes.getNodeAs<CXXCatchStmt>("catch"),
+ *Result.Context);
+}
+
+bool ThrowByValueCatchByReferenceCheck::isFunctionParameter(
+ const DeclRefExpr *declRefExpr) {
+ auto *valueDecl = declRefExpr->getDecl();
+ if (!isa<VarDecl>(valueDecl))
+ return false;
+ auto *varDecl = cast<VarDecl>(valueDecl);
+ return !varDecl->isLocalVarDecl();
+}
+
+bool ThrowByValueCatchByReferenceCheck::isCatchVariable(
+ const DeclRefExpr *declRefExpr) {
+ auto *valueDecl = declRefExpr->getDecl();
+ if (!isa<VarDecl>(valueDecl))
+ return false;
+ auto *varDecl = cast<VarDecl>(valueDecl);
+ return varDecl->isExceptionVariable();
+}
+
+bool ThrowByValueCatchByReferenceCheck::isFunctionOrCatchVar(
+ const DeclRefExpr *declRefExpr)
+{
+ return isFunctionParameter(declRefExpr) || isCatchVariable(declRefExpr);
+}
+
+void ThrowByValueCatchByReferenceCheck::diagnoseThrowLocations(
+ const CXXThrowExpr *throwExpr) {
+ if (!throwExpr)
+ return;
+ auto *subExpr = throwExpr->getSubExpr();
+ if (!subExpr)
+ return;
+ 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"
+ auto *inner = subExpr->IgnoreParenImpCasts();
+ if (isa<StringLiteral>(inner))
+ return;
+ //do not diagnose on catch variables
+ if (isa<DeclRefExpr>(inner)) {
+ auto *declRef = cast<DeclRefExpr>(inner);
+ if (isCatchVariable(declRef))
+ return;
+ }
+ diag(subExpr->getLocStart(), "throw expression throws a pointer; it should "
+ "throw a non pointer value instead");
+ }
+ // 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
+
+ // from CXXThrowExpr, move through all casts until you either encounter an
+ // DeclRefExpr
+ // CXXConstructExpr.
+ // if it's a DeclRefExpr, emit message if the referenced variable is not a catch variable or function parameter
+ // if it's a CopyOrMoveConstructor, emit message if after casts, it
+ // contains a DeclRefExpr for the parameter and that does not reference a catch variable or function parameter
+ // otherwise - do not emit a message.
+ if (checkAnonymousTemporaries) {
+ bool emit = false;
+ auto *currentSubExpr = subExpr->IgnoreImpCasts();
+ const DeclRefExpr *variableReference;
+ if (isa<DeclRefExpr>(currentSubExpr))
+ variableReference = cast<DeclRefExpr>(currentSubExpr);
+ else
+ variableReference = nullptr;
+ const CXXConstructExpr *constructorCall;
+ if (isa<CXXConstructExpr>(currentSubExpr))
+ constructorCall = cast<CXXConstructExpr>(currentSubExpr);
+ else
+ constructorCall = nullptr;
+ // Check for DeclRefExpr that reference neither a function parameter or a
+ // catch variable
+ if (variableReference)
+ emit = !isFunctionOrCatchVar(variableReference);
+ // Check for copy construction
+ // we're now through all potential casts. This should be a
+ // CXXConstructExpr if it's not a variable reference
+ // if it's a copy constructor it could copy from a lvalue
+ else 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)->IgnoreImpCasts();
+ // if the subexpr is now a DeclRefExpr, it's a real variable
+ if (isa<DeclRefExpr>(currentSubExpr))
+ emit = !isFunctionOrCatchVar(cast<DeclRefExpr>(currentSubExpr));
+ }
+ if (emit)
+ diag(subExpr->getLocStart(),
+ "throw expression should throw anonymous temporary values instead");
+ }
+}
+
+void ThrowByValueCatchByReferenceCheck::diagnoseCatchLocations(
+ const CXXCatchStmt *catchStmt, ASTContext &context) {
+ const char *diagMsgCatchReference = "catch handler catches a pointer value; "
+ "should throw a non-pointer value and "
+ "catch by reference instead";
+ if (!catchStmt)
+ return;
+ 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<PointerType>(canonical)->getPointeeType().getTypePtr();
+ if (pointeeType->isAnyCharacterType() == false) {
+ diag(varDecl->getLocStart(), diagMsgCatchReference);
+ }
+ } else if (!type->isReferenceType()) {
+ // not pointer, not reference this means it must be "by value"
+ // do not diagnose on built-in types - catching them by value
+ // is ok
+ if (!caughtType.isTrivialType(context))
+ diag(varDecl->getLocStart(), diagMsgCatchReference);
+ }
+}
+
+} // namespace tidy
+} // namespace clang
Index: clang-tidy/misc/MiscTidyModule.cpp
===================================================================
--- clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tidy/misc/MiscTidyModule.cpp
@@ -25,6 +25,7 @@
#include "SizeofContainerCheck.h"
#include "StaticAssertCheck.h"
#include "SwappedArgumentsCheck.h"
+#include "ThrowByValueCatchByReferenceCheck.h"
#include "UndelegatedConstructor.h"
#include "UniqueptrResetReleaseCheck.h"
#include "UnusedAliasDeclsCheck.h"
@@ -66,6 +67,8 @@
"misc-static-assert");
CheckFactories.registerCheck<SwappedArgumentsCheck>(
"misc-swapped-arguments");
+ CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>(
+ "misc-throw-by-value-catch-by-reference");
CheckFactories.registerCheck<UndelegatedConstructorCheck>(
"misc-undelegated-constructor");
CheckFactories.registerCheck<UniqueptrResetReleaseCheck>(
Index: clang-tidy/misc/CMakeLists.txt
===================================================================
--- clang-tidy/misc/CMakeLists.txt
+++ clang-tidy/misc/CMakeLists.txt
@@ -17,6 +17,7 @@
SizeofContainerCheck.cpp
StaticAssertCheck.cpp
SwappedArgumentsCheck.cpp
+ ThrowByValueCatchByReferenceCheck.cpp
UndelegatedConstructor.cpp
UnusedAliasDeclsCheck.cpp
UnusedParametersCheck.cpp
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
