| Hi, I incorporated most of your changes. There are 2 open issues - on one location I could not follow your advice, the compiler refused to compile the code. I chose a different approach and hope you like it. - the second thing is this MaterializeTemporary advice that you gave. I don’t fully understand it (possibly due to a lack of understanding the AST and a lack of documentation of the proposed methods). Could you please flesh out your idea and why you think it is necessary? Or at least give an example where the current code does not work correctly. Regards Tobias
|
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,160 @@
+//===--- 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) {
+ // This is a C++ only check thus we register the matchers only 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) {
+ return isa<ParmVarDecl>(declRefExpr->getDecl());
+}
+
+bool ThrowByValueCatchByReferenceCheck::isCatchVariable(
+ const DeclRefExpr *declRefExpr) {
+ auto *valueDecl = declRefExpr->getDecl();
+ auto *varDecl = dyn_cast<VarDecl>(valueDecl);
+ return varDecl ? varDecl->isExceptionVariable() : false;
+}
+
+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();
+ if (qualType->isPointerType()) {
+ // the code is throwing a pointer
+ // in case it is strng literal, it is safe and we return
+ auto *inner = subExpr->IgnoreParenImpCasts();
+ if (isa<StringLiteral>(inner))
+ return;
+ // if it's a variable from a catch statement, we return as well
+ auto *declRef = dyn_cast<DeclRefExpr>(inner);
+ if (declRef && isCatchVariable(declRef)) {
+ return;
+ }
+ diag(subExpr->getLocStart(), "throw expression throws a pointer; it should "
+ "throw a non-pointer value instead");
+ }
+ // If the throw statement does not throw by pointer then it throws by value
+ // which is ok.
+ // There are addition checks that emit diagnosis messages if the thrown value
+ // is not an RValue. See:
+ // https://www.securecoding.cert.org/confluence/display/cplusplus/ERR09-CPP.+Throw+anonymous+temporaries
+ // This behavior can be influenced by an option.
+
+ // If we encounter a CXXThrowExpr, we move through all casts until you either
+ // encounter a DeclRefExpr or a CXXConstructExpr.
+ // if it's a DeclRefExpr, we emit a 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 as constructor parameter 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 =
+ dyn_cast<DeclRefExpr>(currentSubExpr);
+ const CXXConstructExpr *constructorCall =
+ dyn_cast<CXXConstructExpr>(currentSubExpr);
+ // If we have a declRefExpr, we flag for emitting a diagnosis message in
+ // case the referenced variable is neither a function parameter nor a
+ // variable declared in the catch statement
+ if (variableReference)
+ emit = !isFunctionOrCatchVar(variableReference);
+ else if (constructorCall &&
+ constructorCall->getConstructor()->isCopyOrMoveConstructor()) {
+ // If we have a copy / move construction, we emit a diagnosis message if
+ // the object that we copy construct from is neither a function parameter
+ // nor a variable declared in a catch statement
+ 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 and we emit a
+ // diagnosis message.
+ if (auto *tmp = dyn_cast<DeclRefExpr>(currentSubExpr))
+ emit = !isFunctionOrCatchVar(tmp);
+ }
+ 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 *varDecl = catchStmt->getExceptionDecl();
+ if (caughtType->isPointerType()) {
+ // We do not diagnose when catching pointer to strings since we also allow
+ // throwing string literals.
+ if (const auto *PT = caughtType.getCanonicalType()->getAs<PointerType>()) {
+ if (!PT->getPointeeType()->isAnyCharacterType())
+ diag(varDecl->getLocStart(), diagMsgCatchReference);
+ }
+ } else if (!caughtType->isReferenceType()) {
+ // If it's not a pointer and not a reference then it must be thrown "by
+ // value". In this case we should emit a diagnosis message unless the type
+ // is trivial.
+ 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
