Re: [PATCH] D11328: [clang-tidy] new "throw-by-value-catch-by-reference" check for clang-tidy

[Tobias Langner via cfe-commits]

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  struct remove_reference  {typedef T type;};
+template  struct remove_reference  {typedef T type;};
+template  struct remove_reference {typedef T type;};
+
+template 
+typename remove_reference::type&& move(T&& arg) {
+  return static_cast::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) {
+  }
+}
+
+typed

Re: [PATCH] D11328: [clang-tidy] new "throw-by-value-catch-by-reference" check for clang-tidy

[Tobias Langner via cfe-commits]

randomcppprogrammer added a reviewer: aaron.ballman.
randomcppprogrammer updated this revision to Diff 34111.
randomcppprogrammer added a comment.

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


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,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) {
+  }
+}
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

Re: [PATCH] D11328: [clang-tidy] new "throw-by-value-catch-by-reference" check for clang-tidy

[Tobias Langner via cfe-commits]

randomcppprogrammer updated this revision to Diff 36795.
randomcppprogrammer marked 11 inline comments as done.
randomcppprogrammer added a comment.

Incorporated feedback from Aaron Ballmann

main changes:

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


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  struct remove_reference  {typedef T type;};
+template  struct remove_reference  {typedef T type;};
+template  struct remove_reference {typedef T type;};
+
+template 
+typename remove_reference::type&& move(T&& arg) {
+  return static_cast::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;

Fwd: [PATCH] D11328: [clang-tidy] new "throw-by-value-catch-by-reference" check for clang-tidy

[Tobias Langner via cfe-commits]

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.RegardsTobiasBegin forwarded message:From: Tobias Langner Date: 8 October 2015 at 00:02:39 GMT+3To: randomcppprogram...@gmail.com, kli...@google.com, cfe-commits@lists.llvm.org, aaron.ball...@gmail.comCc: j...@jbcoe.netSubject: Re: [PATCH] D11328: [clang-tidy] new "throw-by-value-catch-by-reference" check for clang-tidyrandomcppprogrammer updated this revision to Diff 36795.randomcppprogrammer marked 11 inline comments as done.randomcppprogrammer added a comment.Incorporated feedback from Aaron Ballmannmain changes:- replaced isa followed by cast with dyn_cast- reworked comments- fixed typo in diagnosis messagehttp://reviews.llvm.org/D11328Files:  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.cppIndex: 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  struct remove_reference  {typedef T type;};
+template  struct remove_reference  {typedef T type;};
+template  struct remove_reference {typedef T type;};
+
+template 
+typename remove_reference::type&& move(T&& arg) {
+  return static_cast::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

Re: [PATCH] D11328: [clang-tidy] new "throw-by-value-catch-by-reference" check for clang-tidy

[Tobias Langner via cfe-commits]

Hi Aaron,

> 
>> - 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?
> 
yes. I added this to the test (you can find it here if required: 
https://github.com/iso8859-1/clang-tools-extra/blob/tobias-additional-checks/test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp)
 and ran it through clang-query to check all throw statements. I found the 
following patterns:

Throw variants
y   throw->new = pointer from new
y/n throw->ImplicitCast->declrefexpr = throw named variable
n   throw->CXXConstructExpr (non-copy-constructor) = construction with normal 
parameter
n   throw->ImplicitCast->StringLiteral = throw string literal
n   throw->CXXConstructExpr (copy/move)->CallExpr (xvalue) = move
n   throw->CXXConstructExpr (copy/move)->MaterializeTemporary (xvalue)->Call
y   throw->CXXConstructExpr (copy/move)->ImplicitCast->CallExpr (lvalue)
n   throw->CXXConstructExpr (copy/move)->MaterializeTemporary (xvalue)->Call

the y or n or y/n means that we need to diagnose (the y/n depends on whether 
it’s a function parameter or catch parameter or not). When looking at the 
section with the copy construction, you’ll see that it depends on lvalue/xvalue 
(which you probably already knew). But MaterializeTemporary does not detect the 
T&& case.
My current thought is to go for “isLValue()” on Expr. It can be used for the 
CallExpr & the MaterializeTemporaryExpr alike and seems to capture the case 
where we want to diagnose for anonymous temporaries. What do you think?

Tobias

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D11328: [clang-tidy] new "throw-by-value-catch-by-reference" check for clang-tidy

[Tobias Langner via cfe-commits]

randomcppprogrammer updated this revision to Diff 36977.
randomcppprogrammer marked 7 inline comments as done.
randomcppprogrammer added a comment.

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


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,160 @@
+// 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  struct remove_reference  {typedef T type;};
+template  struct remove_reference  {typedef T type;};
+template  struct remove_reference {typedef T type;};
+
+template 
+typename remove_reference::type&& move(T&& arg) {
+  return static_cast::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 (f

Re: [PATCH] D11328: [clang-tidy] new "throw-by-value-catch-by-reference" check for clang-tidy

[Tobias Langner via cfe-commits]

randomcppprogrammer updated this revision to Diff 36983.
randomcppprogrammer added a comment.

fixed formatting


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,156 @@
+// 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  struct remove_reference { typedef T type; };
+template  struct remove_reference { typedef T type; };
+template  struct remove_reference { typedef T type; };
+
+template  typename remove_reference::type &&move(T &&arg) {
+  return static_cast::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 [m

[PATCH] D13620: Documentation for "Throw by value, catch by reference" check in clang-tidy

[Tobias Langner via cfe-commits]

randomcppprogrammer created this revision.
randomcppprogrammer added reviewers: aaron.ballman, cfe-commits.

adds documentation for the check mentioned in the title

http://reviews.llvm.org/D13620

Files:
  docs/clang-tidy/checks/misc-throw-by-value-catch-by-reference.rst

Index: docs/clang-tidy/checks/misc-throw-by-value-catch-by-reference.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-throw-by-value-catch-by-reference.rst
@@ -0,0 +1,11 @@
+misc-throw-by-value-catch-by-reference
+==
+
+Finds violations of the rule "Throw by value, catch by reference" presented 
for example in "C++ Coding Standards" by H. Sutter and A. Alexandrescu. This 
check also has the option to find violations of the rule "Throw anonymous 
temporaries" 
(https://www.securecoding.cert.org/confluence/display/cplusplus/ERR09-CPP.+Throw+anonymous+temporaries).
 The option is named "CheckThrowTemporaries" and it's on by default.
+
+Exceptions:
+- throwing string literals will not be flagged despite being a pointer. They 
are not susceptible to slicing and the usage of string literals is idomatic.
+- catching character pointers (char, wchar_t, unicode character types) will 
not be flagged to allow catching sting literals.
+- moved named values will not be flagged as not throwing an anonymous 
temporary. In this case we can be sure that the user knows that the object 
can't be accessed outside catch blocks handling the error.
+- throwing function parameters will not be flagged as not throwing an 
anonymous temporary. This allows helper functions for throwing.
+- re-throwing caught exception variables will not be flragged as not throwing 
an anonymous temporary. Although this can usually be done by just writing 
"throw;" it happens often enough in real code.


Index: docs/clang-tidy/checks/misc-throw-by-value-catch-by-reference.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-throw-by-value-catch-by-reference.rst
@@ -0,0 +1,11 @@
+misc-throw-by-value-catch-by-reference
+==
+
+Finds violations of the rule "Throw by value, catch by reference" presented for example in "C++ Coding Standards" by H. Sutter and A. Alexandrescu. This check also has the option to find violations of the rule "Throw anonymous temporaries" (https://www.securecoding.cert.org/confluence/display/cplusplus/ERR09-CPP.+Throw+anonymous+temporaries). The option is named "CheckThrowTemporaries" and it's on by default.
+
+Exceptions:
+- throwing string literals will not be flagged despite being a pointer. They are not susceptible to slicing and the usage of string literals is idomatic.
+- catching character pointers (char, wchar_t, unicode character types) will not be flagged to allow catching sting literals.
+- moved named values will not be flagged as not throwing an anonymous temporary. In this case we can be sure that the user knows that the object can't be accessed outside catch blocks handling the error.
+- throwing function parameters will not be flagged as not throwing an anonymous temporary. This allows helper functions for throwing.
+- re-throwing caught exception variables will not be flragged as not throwing an anonymous temporary. Although this can usually be done by just writing "throw;" it happens often enough in real code.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits