[PATCH] D11328: [clang-tidy] new "throw-by-value-catch-by-reference" check for clang-tidy
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Tue Sep 8 06:53:49 PDT 2015
aaron.ballman added a comment.
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.
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:23
@@ +22,3 @@
+ : ClangTidyCheck(Name, Context),
+ checkAnonymousTemporaries(Options.get("CheckThrowTemporaries", "true") ==
+ "true") {}
----------------
Can we just use a bool instead of the string "true"? The constructor for AssertSideEffectCheck does this, for instance.
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:31
@@ +30,3 @@
+
+ Finder->addMatcher(throwExpr().bind("throw"), this);
+ Finder->addMatcher(catchStmt().bind("catch"), this);
----------------
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).
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:43
@@ +42,3 @@
+ const auto *throwExpr = Result.Nodes.getNodeAs<CXXThrowExpr>("throw");
+ if (throwExpr != nullptr) {
+ auto *subExpr = throwExpr->getSubExpr();
----------------
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);
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:44
@@ +43,3 @@
+ if (throwExpr != nullptr) {
+ auto *subExpr = throwExpr->getSubExpr();
+ auto qualType = subExpr->getType();
----------------
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");
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:55
@@ +54,3 @@
+ }
+ diag(subExpr->getLocStart(), "avoid throwing pointer");
+ }
----------------
How about: "throw expression throws a pointer value; should throw a non-pointer value instead"?
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:63
@@ +62,3 @@
+
+ // algorithm
+ // from CXXThrowExpr, move through all casts until you either encounter an
----------------
No need to state that this is an algorithm. The reader can likely guess as much. ;-)
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:74
@@ +73,3 @@
+ auto *currentSubExpr = subExpr;
+ while (isa<CastExpr>(currentSubExpr)) {
+ auto *currentCast = cast<CastExpr>(currentSubExpr);
----------------
I think we want IgnoreParenImpCasts() here as well.
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:98
@@ +97,3 @@
+ auto *currentSubExpr = *argIter;
+ while (isa<CastExpr>(currentSubExpr)) {
+ auto *currentCast = cast<CastExpr>(currentSubExpr);
----------------
IgnoreParenImpCasts(), again?
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:102
@@ +101,3 @@
+ }
+ // if the subexpr is now a DeclRefExpr, it's a real variable
+ if (isa<DeclRefExpr>(currentSubExpr))
----------------
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
}
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:108
@@ +107,3 @@
+ if (emit)
+ diag(subExpr->getLocStart(), "prefer throwing anonymous temporaries");
+ }
----------------
How about: "throw expression should throw an anonymous temporary value instead"?
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:112
@@ +111,3 @@
+ const auto *catchStmt = Result.Nodes.getNodeAs<CXXCatchStmt>("catch");
+ if (catchStmt != nullptr) {
+ auto caughtType = catchStmt->getCaughtType();
----------------
No need for the != nullptr.
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:119
@@ +118,3 @@
+ if (type->isPointerType()) {
+ auto canonical = type->getCanonicalTypeInternal().getTypePtr();
+ // check if pointed-to-type is char, wchar_t, char16_t, char32_t
----------------
Should use getCanonicalType() instead of the internal version.
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:124
@@ +123,3 @@
+ if (pointeeType->isAnyCharacterType() == false) {
+ diag(varDecl->getLocStart(), "catch by reference");
+ }
----------------
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).
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:128
@@ +127,3 @@
+ // not pointer, not reference this means it must be "by value"
+ // do not advice on built-in types - catching them by value
+ // is ok
----------------
"do not advice" -> "do not diagnose"
================
Comment at: test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp:3
@@ +2,3 @@
+
+//#include <utility>
+
----------------
Should remove this.
================
Comment at: test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp:19
@@ +18,3 @@
+ throw new logic_error("by pointer");
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: avoid throwing pointer
+ // [misc-throw-by-value-catch-by-reference]
----------------
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).
================
Comment at: test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp:44
@@ +43,3 @@
+ // can be enabled once std::move can be included
+ // throw std::move(lvalue)
+ int &ex = lastException;
----------------
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);
}
================
Comment at: test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp:53
@@ +52,3 @@
+void throwReferenceFunc(logic_error &ref) {
+ throw ref;
+ // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: prefer throwing anonymous
----------------
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.
================
Comment at: test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp:138
@@ +137,2 @@
+ }
+}
----------------
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
}
http://reviews.llvm.org/D11328
More information about the cfe-commits
mailing list