[PATCH] D11328: [clang-tidy] new "throw-by-value-catch-by-reference" check for clang-tidy
Tobias Langner
randomcppprogrammer at gmail.com
Sat Jul 25 01:33:16 PDT 2015
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 <aaron at aaronballman.com> wrote:
>
> On Sat, Jul 18, 2015 at 11:44 AM, Tobias Langner
> <randomcppprogrammer at gmail.com> 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
More information about the cfe-commits
mailing list