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

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

On Wed, Oct 7, 2015 at 5:08 PM, Tobias Langner <randomcppprogrammer at gmail.com> 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?


================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:55
@@ +54,3 @@
+  auto *varDecl = dyn_cast<VarDecl>(valueDecl);
+  return varDecl ? varDecl->isExceptionVariable() : false;
+}
----------------
How about:
```
if (const auto *VD = dyn_cast<VarDecl>(declRefExpr->getDecl()))
  return VD->isExceptionVariable();
return false;
```

================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:72
@@ +71,3 @@
+  if (qualType->isPointerType()) {
+    // the code is throwing a pointer
+    // in case it is strng literal, it is safe and we return
----------------
Sentence capitalization and punctuation in the comment.

================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:77
@@ +76,3 @@
+      return;
+    // if it's a variable from a catch statement, we return as well
+    auto *declRef = dyn_cast<DeclRefExpr>(inner);
----------------
Sentence capitalization and punctuation in the comment.

================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:94
@@ +93,3 @@
+  // 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
----------------
Sentence capitalization and punctuation in the comment.

================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:121
@@ +120,3 @@
+      auto *currentSubExpr = (*argIter)->IgnoreImpCasts();
+      // if the subexpr is now a DeclRefExpr, it's a real variable and we emit a
+      // diagnosis message.
----------------
Sentence capitalization and punctuation in the comment.

================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:143
@@ +142,3 @@
+  auto *varDecl = catchStmt->getExceptionDecl();
+  if (caughtType->isPointerType()) {
+    // We do not diagnose when catching pointer to strings since we also allow
----------------
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.

================
Comment at: test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp:45
@@ +44,3 @@
+  
+  // can be enabled once std::move can be included
+  throw move(lvalue);  
----------------
This comment is outdated.


http://reviews.llvm.org/D11328