Hi, I modified the code. During this, additional questions popped up that I’d like to resolve before I put up a new patch. Please check my comments below.
> On 20 Jul 2015, at 17:17, Aaron Ballman <[email protected]> wrote: > > On Sat, Jul 18, 2015 at 11:44 AM, Tobias Langner > <[email protected]> wrote: >> randomcppprogrammer created this revision. >> randomcppprogrammer added reviewers: klimek, cfe-commits, jbcoe. >> >> This patch contains an implementation to check whether exceptions where >> thrown by value and caught by const reference. This is a guideline mentioned >> in different places, e.g. "C++ Coding Standards" by H. Sutter. > > I happened to be working on this exact same problem while developing > rules for CERT's C++ secure coding guidelines, using a very similar > approach. Thank you for looking into this! If you'd like another > coding standard to compare this to: > https://www.securecoding.cert.org/confluence/display/cplusplus/ERR61-CPP.+Catch+exceptions+by+lvalue+reference > (we have a recommendation for throwing rvalues as well). > > A few things I noticed: > > This is not testing whether you are throwing by value, but are instead > testing whether you are throwing a pointer. Is this intentional? Yes. It’s easier to test and since you can only throw by pointer or by value, it’s in my opinion equivalent. > Also, > not all thrown pointers are dangerous. For instance: throw "This is a > cheap exception"; is very reasonable code, while throw new > BadIdea(12); is not. this can be handled by allowing StringLiterals to be thrown, but no other pointers. My problem with this is that I don’t know how to recognise this at the catch statement without removing the check for pointers. The only idea that I have right now is to check whether the pointed to type is ‘const char’ or ‘const wchar_t’ or one of the new ones for unicode. But it feels a little bit brittle. > > As for catching, catching by const lvalue reference is not always the > correct recommendation. For instance, the catch block could be > modifying the exception object and rethrowing it, at which point you > cannot catch by const reference. Also, not all types really need to be > caught by reference, such as builtin types (think int) or trivial > types. I added the check for builtin & trivial types - that was a good idea. Regarding catching as reference (opposed to const reference), I could think of 2 different options: - require nolint - which I think is viable. It marks in source code that someone had thought about this potential problem. - having an option for the test (something like enforce const reference). Both have their benefits & drawbacks. If a project requires at most call sites that the exception is modified, the second one is better - but I don’t know how to test different option given the current test infrastructure. If modifying an exception is rare, I think the first option is better as it forces to comment this rare situation. I would not like to remove the test for const-ness on catch because it can be a real performance hog that some might not be aware about. Any advice which way to go? > > Some comments on the code below. > >> >> Patch by Tobias Langner >> >> http://reviews.llvm.org/D11328 >> >> Files: >> clang-tidy/misc/CMakeLists.txt >> clang-tidy/misc/MiscTidyModule.cpp >> clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp >> clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h >> test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp > >> Index: test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp >> =================================================================== >> --- /dev/null >> +++ test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp >> @@ -0,0 +1,71 @@ >> +// RUN: $(dirname %s)/check_clang_tidy.sh %s >> misc-throw-by-value-catch-by-reference %t >> +// REQUIRES: shell >> + >> +class logic_error { >> +public: >> + logic_error(const char *message) {} >> +}; >> + >> +int selector; > > Don't need this. Done > >> +typedef logic_error *logic_ptr; >> +typedef logic_ptr logic_double_typedef; >> + >> +void testThrowFunc() { >> + switch (selector) { > > Or the switch. Done > >> + case 0: >> + throw new logic_error("by pointer"); >> + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: avoid throwing pointer >> + // [misc-throw-by-value-catch-by-reference] >> + case 1: { >> + logic_ptr tmp = new logic_error("by pointer"); >> + throw tmp; >> + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: avoid throwing pointer >> + // [misc-throw-by-value-catch-by-reference] >> + } >> + case 2: >> + throw logic_error("by value"); >> + } >> +} >> + >> +void catchByPointer() { >> + try { >> + testThrowFunc(); >> + } catch (logic_error *e) { >> + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch by const reference >> + // [misc-throw-by-value-catch-by-reference] >> + } >> +} >> + >> +void catchByValue() { >> + try { >> + testThrowFunc(); >> + } catch (logic_error e) { >> + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch by const reference >> + // [misc-throw-by-value-catch-by-reference] >> + } >> +} >> + >> +void catchByReference() { >> + try { >> + testThrowFunc(); >> + } catch (logic_error &e) { >> + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch by const reference to >> + // avoid unnecessary copies [misc-throw-by-value-catch-by-reference] >> + } >> +} >> + >> +void catchByConstReference() { >> + try { >> + testThrowFunc(); >> + } catch (const logic_error &e) { >> + } >> +} >> + >> +void catchTypedef() { >> + try { >> + testThrowFunc(); >> + } catch (logic_ptr e) { >> + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: catch by const reference >> + // [misc-throw-by-value-catch-by-reference] >> + } >> +} >> Index: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h >> =================================================================== >> --- /dev/null >> +++ clang-tidy/misc/ThrowByValueCatchByReferenceCheck.h >> @@ -0,0 +1,32 @@ >> +//===--- ThrowByValueCatchByReferenceCheck.h - clang-tidy--------*- C++ >> -*-===// >> +// >> +// The LLVM Compiler Infrastructure >> +// >> +// This file is distributed under the University of Illinois Open Source >> +// License. See LICENSE.TXT for details. >> +// >> +//===----------------------------------------------------------------------===// >> + >> +#ifndef >> LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H >> +#define >> LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H >> + >> +#include "../ClangTidy.h" >> + >> +namespace clang { >> +namespace tidy { >> + >> +///\brief checks for locations that do not throw by value >> +// or catch by reference. >> +class ThrowByValueCatchByReferenceCheck : public ClangTidyCheck { >> +public: > > Since everything's public, perhaps just use a struct instead? for me structs are close to pods (and some static analysis tools like QAC++ actually have rules that enforce that). This is clearly not a pod so I think class is appropriate (unless that’s against the coding guidelines of the clang project). > >> + ThrowByValueCatchByReferenceCheck(StringRef Name, ClangTidyContext >> *Context) >> + : ClangTidyCheck(Name, Context) {} >> + void registerMatchers(ast_matchers::MatchFinder *Finder) override; >> + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; >> +}; >> + >> +} // namespace tidy >> +} // namespace clang >> + >> +#endif // >> LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_THROW_BY_VALUE_CATCH_BY_REFERENCE_H >> + >> Index: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp >> =================================================================== >> --- /dev/null >> +++ clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp >> @@ -0,0 +1,57 @@ >> +//===--- ThrowByValueCatchByReferenceCheck.cpp - >> clang-tidy----------------===// >> +// >> +// The LLVM Compiler Infrastructure >> +// >> +// This file is distributed under the University of Illinois Open Source >> +// License. See LICENSE.TXT for details. >> +// >> +//===----------------------------------------------------------------------===// >> + >> +#include "ThrowByValueCatchByReferenceCheck.h" >> +#include "clang/AST/ASTContext.h" >> +#include "clang/ASTMatchers/ASTMatchFinder.h" >> + >> +using namespace clang::ast_matchers; >> + >> +namespace clang { >> +namespace tidy { >> + >> +void ThrowByValueCatchByReferenceCheck::registerMatchers(MatchFinder >> *Finder) { >> + Finder->addMatcher(throwExpr().bind("throw"), this); >> + Finder->addMatcher(catchStmt().bind("catch"), this); >> +} >> + >> +void ThrowByValueCatchByReferenceCheck::check( >> + const MatchFinder::MatchResult &Result) { >> + const auto *throwExpr = Result.Nodes.getNodeAs<CXXThrowExpr>("throw"); >> + if (throwExpr != nullptr) { >> + auto *subExpr = throwExpr->getSubExpr(); >> + auto qualType = subExpr->getType(); >> + auto *type = qualType.getTypePtr(); >> + if (type->isPointerType() == true) { // throwing a pointer > > Should drop the ==; also, elide the braces to match the coding conventions. done > >> + diag(subExpr->getLocStart(), "avoid throwing pointer"); >> + } >> + } >> + const auto *catchStmt = Result.Nodes.getNodeAs<CXXCatchStmt>("catch"); >> + if (catchStmt != nullptr) { >> + auto caughtType = catchStmt->getCaughtType(); >> + auto *type = caughtType.getTypePtr(); >> + auto *varDecl = catchStmt->getExceptionDecl(); > > Not all catch clauses have a type, such as catch (...). Should guard > against type being null, and include a test case for this. That also > goes for varDecl, so you may want to make the relationship more clear > that if one is null, they both are null. done > >> + if (type->isPointerType()) { >> + diag(varDecl->getLocStart(), >> + "catch by const reference"); >> + } else if (!type->isReferenceType()) { >> + diag(varDecl->getLocStart(), "catch by const reference"); >> + } else { >> + auto* reference = type->castAs<const ReferenceType>(); >> + assert(reference!=nullptr); >> + if (reference->getPointeeType().isConstQualified() == false) { >> + diag(varDecl->getLocStart(), >> + "catch by const reference to avoid unnecessary copies"); >> + } >> + } >> + } >> +} >> + >> +} // namespace tidy >> +} // namespace clang >> Index: clang-tidy/misc/MiscTidyModule.cpp >> =================================================================== >> --- clang-tidy/misc/MiscTidyModule.cpp >> +++ clang-tidy/misc/MiscTidyModule.cpp >> @@ -21,6 +21,7 @@ >> #include "NoexceptMoveConstructorCheck.h" >> #include "StaticAssertCheck.h" >> #include "SwappedArgumentsCheck.h" >> +#include "ThrowByValueCatchByReferenceCheck.h" >> #include "UndelegatedConstructor.h" >> #include "UniqueptrResetReleaseCheck.h" >> #include "UnusedRAIICheck.h" >> @@ -54,6 +55,8 @@ >> "misc-static-assert"); >> CheckFactories.registerCheck<SwappedArgumentsCheck>( >> "misc-swapped-arguments"); >> + CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>( >> + "misc-throw-by-value-catch-by-reference"); >> CheckFactories.registerCheck<UndelegatedConstructorCheck>( >> "misc-undelegated-constructor"); >> CheckFactories.registerCheck<UniqueptrResetReleaseCheck>( >> Index: clang-tidy/misc/CMakeLists.txt >> =================================================================== >> --- clang-tidy/misc/CMakeLists.txt >> +++ clang-tidy/misc/CMakeLists.txt >> @@ -13,6 +13,7 @@ >> NoexceptMoveConstructorCheck.cpp >> StaticAssertCheck.cpp >> SwappedArgumentsCheck.cpp >> + ThrowByValueCatchByReferenceCheck.cpp >> UndelegatedConstructor.cpp >> UnusedRAIICheck.cpp >> UniqueptrResetReleaseCheck.cpp >> > > ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
