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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
