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

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!


================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:46
@@ +45,3 @@
+
+bool ThrowByValueCatchByReferenceCheck::isFunctionParameter(
+    const DeclRefExpr *declRefExpr) {
----------------
Judging by the function name, I think the code should actually be:
```
return isa<ParmVarDecl>(declRefExpr->getDecl());
```

================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:60
@@ +59,3 @@
+    return false;
+  auto *varDecl = cast<VarDecl>(valueDecl);
+  return varDecl->isExceptionVariable();
----------------
Instead of isa<> followed by cast<>, the more idiomatic approach is:
```
if (const auto *VD = dyn_cast<VarDecl>(declRefExpr->getDecl()))
  return VD->isExceptionVariable();
```


================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:66
@@ +65,3 @@
+    const DeclRefExpr *declRefExpr)
+{
+ return isFunctionParameter(declRefExpr) || isCatchVariable(declRefExpr); 
----------------
Should run clang-format over this; the brace should go with the closing paren.

================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:78
@@ +77,3 @@
+  auto qualType = subExpr->getType();
+  auto *type = qualType.getTypePtr();
+  if (type->isPointerType()) {
----------------
No need to call getTypePtr(); QualType objects are thin wrappers around a Type *. So you can do qualType->isPointerType(), for instance.

================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:86
@@ +85,3 @@
+      return;
+    //do not diagnose on catch variables
+    if (isa<DeclRefExpr>(inner)) {
----------------
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.

================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:88
@@ +87,3 @@
+    if (isa<DeclRefExpr>(inner)) {
+      auto *declRef = cast<DeclRefExpr>(inner);
+      if (isCatchVariable(declRef))
----------------
Please use dyn_cast<> instead of isa<> followed by cast<>. Applies here and elsewhere in the patch.

================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:93
@@ +92,3 @@
+    diag(subExpr->getLocStart(), "throw expression throws a pointer; it should "
+                                 "throw a non pointer value instead");
+  }
----------------
non-pointer, please

================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:111
@@ +110,3 @@
+    auto *currentSubExpr = subExpr->IgnoreImpCasts();
+    const DeclRefExpr *variableReference;
+    if (isa<DeclRefExpr>(currentSubExpr))
----------------
This should be:
```
if (const auto *variableReference = dyn_cast<DeclRefExpr>(currentSubExpr)) {
} else if (const auto *constructorCall = dyn_cast<CXXConstructExpr>(currentSubExpr)) {
}
```

================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:129
@@ +128,3 @@
+    // if it's a copy constructor it could copy from a lvalue
+    else if (constructorCall &&
+             constructorCall->getConstructor()->isCopyOrMoveConstructor()) {
----------------
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.)

================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:156
@@ +155,3 @@
+    return;
+  auto *type = caughtType.getTypePtr();
+  auto *varDecl = catchStmt->getExceptionDecl();
----------------
No need to call getTypePtr().

================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:159
@@ +158,3 @@
+  if (type->isPointerType()) {
+    auto canonical = type->getCanonicalTypeInternal().getTypePtr();
+    // check if pointed-to-type is char, wchar_t, char16_t, char32_t
----------------
Same here.

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

================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:162
@@ +161,3 @@
+    auto *pointeeType =
+        cast<PointerType>(canonical)->getPointeeType().getTypePtr();
+    if (pointeeType->isAnyCharacterType() == false) {
----------------
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.

================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h:43
@@ +42,3 @@
+  bool isFunctionOrCatchVar(const DeclRefExpr *declRefExpr);
+  const bool checkAnonymousTemporaries;
+};
----------------
Should be Check instead of check per style guidelines.


http://reviews.llvm.org/D11328