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