[PATCH] D11328: [clang-tidy] new "throw-by-value-catch-by-reference" check for clang-tidy

[Tobias Langner via cfe-commits]

Hi Aaron,

> 
>> - the second thing is this MaterializeTemporary advice that you gave. I don’t fully understand it (possibly due to a lack of understanding the AST and a lack of documentation of the proposed methods). Could you please flesh out your idea and why you think it is necessary? Or at least give an example where the current code does not work correctly.
> 
> 
> Consider code like:
> 
>  struct S {};
> 
>  S& g();
>  S h();
> 
>  void f() {
>    throw g(); // Should diagnose, does not currently
>    throw h(); // Should not diagnose, does not currently
>  }
> 
> "throw g();" isn't throwing a temporary because it's throwing from an lvalue reference object (so the user may have a catch clause expecting the caught object to be the same lvalue reference as what g() returns, except that's not actually the case). If you instead check to see whether the throw expression requires a temporary to be materialized, you'll find that g() will diagnose, while h() still will not.
> 
> Does that help?
> 
yes. I added this to the test (you can find it here if required: https://github.com/iso8859-1/clang-tools-extra/blob/tobias-additional-checks/test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp) and ran it through clang-query to check all throw statements. I found the following patterns:

Throw variants
y   throw->new = pointer from new
y/n throw->ImplicitCast->declrefexpr = throw named variable
n   throw->CXXConstructExpr (non-copy-constructor) = construction with normal parameter
n   throw->ImplicitCast->StringLiteral = throw string literal
n   throw->CXXConstructExpr (copy/move)->CallExpr (xvalue) = move
n   throw->CXXConstructExpr (copy/move)->MaterializeTemporary (xvalue)->Call
y   throw->CXXConstructExpr (copy/move)->ImplicitCast->CallExpr (lvalue)
n   throw->CXXConstructExpr (copy/move)->MaterializeTemporary (xvalue)->Call

the y or n or y/n means that we need to diagnose (the y/n depends on whether it’s a function parameter or catch parameter or not). When looking at the section with the copy construction, you’ll see that it depends on lvalue/xvalue (which you probably already knew). But MaterializeTemporary does not detect the T&& case.
My current thought is to go for “isLValue()” on Expr. It can be used for the CallExpr & the MaterializeTemporaryExpr alike and seems to capture the case where we want to diagnose for anonymous temporaries. What do you think?

Tobias