aaron.ballman added a comment.
On Wed, Oct 7, 2015 at 5:08 PM, Tobias Langner <randomcppprogram...@gmail.com>
wrote:
> Hi,
>
> I incorporated most of your changes. There are 2 open issues
Thank you for your work on this! There are a few minor nitpicks still, but is
definitely moving forward nicely.
> - on one location I could not follow your advice, the compiler refused to
> compile the code. I chose a different approach and hope you like it.
> - 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?
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:55
@@ +54,3 @@
+ auto *varDecl = dyn_cast<VarDecl>(valueDecl);
+ return varDecl ? varDecl->isExceptionVariable() : false;
+}
----------------
How about:
```
if (const auto *VD = dyn_cast<VarDecl>(declRefExpr->getDecl()))
return VD->isExceptionVariable();
return false;
```
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:72
@@ +71,3 @@
+ if (qualType->isPointerType()) {
+ // the code is throwing a pointer
+ // in case it is strng literal, it is safe and we return
----------------
Sentence capitalization and punctuation in the comment.
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:77
@@ +76,3 @@
+ return;
+ // if it's a variable from a catch statement, we return as well
+ auto *declRef = dyn_cast<DeclRefExpr>(inner);
----------------
Sentence capitalization and punctuation in the comment.
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:94
@@ +93,3 @@
+ // encounter a DeclRefExpr or a CXXConstructExpr.
+ // if it's a DeclRefExpr, we emit a message if the referenced variable is not
+ // a catch variable or function parameter
----------------
Sentence capitalization and punctuation in the comment.
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:121
@@ +120,3 @@
+ auto *currentSubExpr = (*argIter)->IgnoreImpCasts();
+ // if the subexpr is now a DeclRefExpr, it's a real variable and we emit
a
+ // diagnosis message.
----------------
Sentence capitalization and punctuation in the comment.
================
Comment at: clang-tidy/misc/ThrowByValueCatchByReferenceCheck.cpp:143
@@ +142,3 @@
+ auto *varDecl = catchStmt->getExceptionDecl();
+ if (caughtType->isPointerType()) {
+ // We do not diagnose when catching pointer to strings since we also allow
----------------
I think this would be better as:
```
if (const auto *PT = caughtType.getCanonicalType()->getAs<PointerType>()) {
if (...)
diag(...);
} else if (!caughtType->isReferenceType() && !caughtType.isTrivialType(context))
diag(...);
```
As-written, it currently checks whether the caught type is a pointer, but not
whether the canonical caught type is a pointer.
================
Comment at: test/clang-tidy/misc-throw-by-value-catch-by-reference.cpp:45
@@ +44,3 @@
+
+ // can be enabled once std::move can be included
+ throw move(lvalue);
----------------
This comment is outdated.
http://reviews.llvm.org/D11328
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
