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