[clang-tidy] new "throw-by-value-catch-by-reference" check for clang-tidy

[randomcppprogrammer]

Summary

This patch contains an implementation to check whether exceptions where thrown by value and caught by const reference. This is a guideline mentioned in different places, e.g. "C++ Coding Standards" by H. Sutter.

Patch by Tobias Langner

[randomcppprogrammer]

Updated check according to comments given by Aaron Ballman. Most notable change: added optional check for throwing anonmyous temporaries.

[aaron.ballman]

This is looking great! Some mostly minor feedback below. The only things I noticed that may require further consideration are throwing function parameters and throwing exception handler variables -- I don't think those scenarios should emit a diagnostic. The former is a bit chatty because of error handling functions, and the latter is correct regardless of the type being thrown (though is a bit silly because the user should really just use throw; instead of rethrowing the variable).

General nitpick: please make sure all comments in the code and tests have proper capitalization, punctuation, etc.

clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp

24

Can we just use a bool instead of the string "true"? The constructor for AssertSideEffectCheck does this, for instance.

32

I'm not certain whether this is feasible or not, but if you can check the options by this point, you could replace a lot of custom AST logic from check() by registering a different matcher if care about anonymous temporaries. For instance:

if (checkAnonymousTemporaries)
  Finder->addMatcher(throwExpr(unless(anyOf(hasDescendant(anyOf(declRefExpr(to(parmVarDecl())), declRefExpr(to(varDecl(isExceptionVar()))), constructExpr(hasDescendant(materializeTemporaryExpr())), expr(hasType(hasCanonicalType(builtinType()))))), unless(has(expr()))))), this);
else
  Finder->addMatcher(throwExpr(), this);

(Leaving the bindings out, but you get the idea).

44

No need for the != nullptr.

May want to consider splitting the throw and catch logic into helper methods to reduce indenting. e.g.,

if (throwExpr)
  doThrowStuff(throwExpr);

if (catchExpr)
  doCatchStuff(catchExpr);

45

I think we want throwExpr->getSubExpr()->IgnoreParenImpCasts(). This looks through *all* implicit casts (and paren expressions, etc), so you can get rid of the if (isa<ImplicitCastExpr>) from below and just look at isa<StringLiteral>. This will then allow code like the following to be properly checked: throw ("I am not a kind person");

56

How about: "throw expression throws a pointer value; should throw a non-pointer value instead"?

64

No need to state that this is an algorithm. The reader can likely guess as much. ;-)

75

I think we want IgnoreParenImpCasts() here as well.

99

IgnoreParenImpCasts(), again?

103

I think the correct way to do this is check to see whether the constructor is passed a MaterializeTemporaryExpr. and if so, we don't want to emit the diagnostic (otherwise we do).

Also, this code does not look to see whether there is a DeclRefExpr that refers to a parameter variable declaration. In my testing, this accounted for a sizable number of false positives because of code like:

void handle_error(const someObj &o) {
  throw o; // Shouldn't diagnose, this isn't dangerous
}

109

How about: "throw expression should throw an anonymous temporary value instead"?

113

No need for the != nullptr.

120

Should use getCanonicalType() instead of the internal version.

125

How about: "catch handler catches a pointer value; should throw a non-pointer value and catch by reference instead"?

May also want to consider combining with the same diagnostic below (or, at the very least, only have the diagnostic text written once and referred to from both places).

129

"do not advice" -> "do not diagnose"

test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp

4

Should remove this.

20

Don't worry about 80-col limits for diagnostic messages -- keep the entire diagnostic on one line (it makes it easier to read the tests).

45

You can add your own move definition to the file:

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);
}

54

I don't think we should diagnose on this case; I found it caused a lot of false positives for code bases that wrap throws in an error handling function.

139

I'd also like to see tests for:

typedef logic_error& fine;
try {
} catch (int i) { // ok
  throw i; // ok
} catch (fine e) { // ok
  throw e; // ok
} catch (logic_error *e) { // diagnose for catching a pointer
  throw e; // ok, despite throwing a pointer
} catch(...) { // ok
  throw; // ok
}

[randomcppprogrammer]

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)

[aaron.ballman]

This patch is looking much closer! Thank you for continuing to work on it.

I found several mechanical changes that need to be made, like style, comments, formatting, isa<> followed by cast<>, etc. There is one logic issue regarding materialized temporaries that I think still needs to be addressed, but otherwise, this patch is getting close!

clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp

47

Judging by the function name, I think the code should actually be:

return isa<ParmVarDecl>(declRefExpr->getDecl());

61

Instead of isa<> followed by cast<>, the more idiomatic approach is:

if (const auto *VD = dyn_cast<VarDecl>(declRefExpr->getDecl()))
  return VD->isExceptionVariable();

67

Should run clang-format over this; the brace should go with the closing paren.

79

No need to call getTypePtr(); QualType objects are thin wrappers around a Type *. So you can do qualType->isPointerType(), for instance.

87

Space between the comment and the start of the sentence; also, comments should be full sentences (with capitalization and punctuation). Applies here and elsewhere in the patch.

89

Please use dyn_cast<> instead of isa<> followed by cast<>. Applies here and elsewhere in the patch.

94

non-pointer, please

112

This should be:

if (const auto *variableReference = dyn_cast<DeclRefExpr>(currentSubExpr)) {
} else if (const auto *constructorCall = dyn_cast<CXXConstructExpr>(currentSubExpr)) {
}

130

I think that what we need to model here is: constructExpr(hasDescendant(materializeTemporaryExpr())) and expr(hasType(hasCanonicalType(builtinType())))

If the throw expression materializes a temporary, or it throws a builtin type, we are good. (Note, the checking for function parameter or catch variable is already done properly.)

157

No need to call getTypePtr().

160

Same here.

Also, please do not use getCanonicalTypeInternal(), you should use getCanonicalType() instead.

163

A cleaner way to do this would be:

if (const auto *PT = getCanonicalType(caughtType)->getAs<PointerType>()) {
  if (!PT->getPointeeType()->isAnyCharacterType())
    diag(...);
}

This also corrects direct comparison against a Boolean on the next line.

clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h

44

Should be Check instead of check per style guidelines.

[randomcppprogrammer]

Incorporated feedback from Aaron Ballmann

main changes:

• replaced isa followed by cast with dyn_cast
• reworked comments
• fixed typo in diagnosis message

[aaron.ballman]

On Wed, Oct 7, 2015 at 5:08 PM, Tobias Langner <[e-mail]> wrote:

Hi,

I incorporated most of your changes. There are 2 open issues

Thank you for your work on this! There are a few minor nitpicks still, but is definitely moving forward nicely.

• 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.

Consider code like:

struct S {};

S& g();
S h();

void f() {
  throw g(); // Should diagnose, does not currently
  throw h(); // Should not diagnose, does not currently
}

"throw g();" isn't throwing a temporary because it's throwing from an lvalue reference object (so the user may have a catch clause expecting the caught object to be the same lvalue reference as what g() returns, except that's not actually the case). If you instead check to see whether the throw expression requires a temporary to be materialized, you'll find that g() will diagnose, while h() still will not.

Does that help?

clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp

56

How about:

if (const auto *VD = dyn_cast<VarDecl>(declRefExpr->getDecl()))
  return VD->isExceptionVariable();
return false;

73

Sentence capitalization and punctuation in the comment.

78

Sentence capitalization and punctuation in the comment.

95

Sentence capitalization and punctuation in the comment.

122

Sentence capitalization and punctuation in the comment.

144

I think this would be better as:

if (const auto *PT = caughtType.getCanonicalType()->getAs<PointerType>()) {
  if (...)
    diag(...);
} else if (!caughtType->isReferenceType() && !caughtType.isTrivialType(context))
  diag(...);

As-written, it currently checks whether the caught type is a pointer, but not whether the canonical caught type is a pointer.

test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp

46

This comment is outdated.

[randomcppprogrammer]

new:

• diagnosis on throwing lvalues returned by function calls such as

struct S;
S& function();

void foo() {

throw function();

}

• updated tests for this case
• updated some comments

[aaron.ballman]

LGTM! Please run your patch through clang-format before committing (just to fix the last bits of style and whitespace issues; no additional review required).

Thank you for your hard work on this! As a cleanup once this goes in: we should add some user-facing documentation to docs\clang-tidy\checks for it. We can even take what's in ThrowByValueCatchByReferenceCheck.h and use that (then link to it from the header file instead of duplicating the words).

[randomcppprogrammer]

fixed formatting

[aaron.ballman]

I have commit in r249899. Thanks again!

clang-tidy/misc/CMakeLists.txt

add_clang_library(clangTidyMiscModule
   MiscTidyModule.cpp
   MoveConstructorInitCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   NonCopyableObjects.cpp
   SizeofContainerCheck.cpp
   StaticAssertCheck.cpp
   SwappedArgumentsCheck.cpp
+  ThrowByValueCatchByReferenceCheck.cpp
   UndelegatedConstructor.cpp
   UnusedAliasDeclsCheck.cpp
   UnusedParametersCheck.cpp
   UnusedRAIICheck.cpp
   UniqueptrResetReleaseCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/misc/MiscTidyModule.cpp

 #include "MacroRepeatedSideEffectsCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
 #include "SizeofContainerCheck.h"
 #include "StaticAssertCheck.h"
 #include "SwappedArgumentsCheck.h"
+#include "ThrowByValueCatchByReferenceCheck.h"
 #include "UndelegatedConstructor.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
 #include "UnusedRAIICheck.h"
 
 namespace clang {
 namespace tidy {
CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "misc-noexcept-move-constructor");
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");
     CheckFactories.registerCheck<SizeofContainerCheck>("misc-sizeof-container");
     CheckFactories.registerCheck<StaticAssertCheck>(
         "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>(
         "misc-uniqueptr-reset-release");
     CheckFactories.registerCheck<UnusedAliasDeclsCheck>(
         "misc-unused-alias-decls");
     CheckFactories.registerCheck<UnusedParametersCheck>(
         "misc-unused-parameters");

clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h

+//===--- 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

clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp

+//===--- 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();
+  if (auto *varDecl = dyn_cast<VarDecl>(valueDecl))
+    return varDecl->isExceptionVariable();
+  return 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.
+  // When encountering a CopyOrMoveConstructor: emit message if after casts,
+  // the expression is a LValue
+  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 (currentSubExpr->isLValue()) {
+        if (auto *tmp = dyn_cast<DeclRefExpr>(currentSubExpr))
+          emit = !isFunctionOrCatchVar(tmp);
+        else if (isa<CallExpr>(currentSubExpr))
+          emit = true;
+      }
+    }
+    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 (const auto *PT = caughtType.getCanonicalType()->getAs<PointerType>()) {
+    // We do not diagnose when catching pointer to strings since we also allow
+    // throwing string literals.
+    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

test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp

+// 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]
+
+  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
+  }
+}
+
+struct S {};
+
+S &returnByReference();
+S returnByValue();
+
+void f() {
+  throw returnByReference(); // Should diagnose
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: throw expression should throw anonymous temporary values instead [misc-throw-by-value-catch-by-reference]
+  throw returnByValue(); // Should not diagnose
+}