On Fri, Oct 9, 2015 at 3:31 PM, Tobias Langner
<randomcppprogram...@gmail.com> wrote:
> 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?
That seems more comprehensive than what I was originally going for, so
I like it. I think that's a good approach, since an xvalue is an
eXpiring value (aka, temporary as far as this check is concerned), and
a prvalue is a pure rvalue (which is any rvalue that isn't an xvalue),
this should cover the cases we care about.
Thanks, great catch!
~Aaron
>
> Tobias
>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
