aaron.ballman added a comment.
In https://reviews.llvm.org/D22910#601083, @Abpostelnicu wrote:
> Can someone please show me an example on how to write a test for this patch?
You could test this with anything that diagnoses side effects in an unevaluated
context (look for `warn_side_effects_une
Abpostelnicu added a comment.
Can someone please show me an example on how to write a test for this patch?
https://reviews.llvm.org/D22910
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-com
Abpostelnicu updated this revision to Diff 73766.
Abpostelnicu marked 3 inline comments as done.
https://reviews.llvm.org/D22910
Files:
lib/AST/Expr.cpp
Index: lib/AST/Expr.cpp
===
--- lib/AST/Expr.cpp
+++ lib/AST/Expr.cpp
@@ -28
aaron.ballman added a comment.
This patch is missing tests for the new functionality.
> aaron.ballman wrote in ExprCXX.h:109
> Missing the full stop at the end of the sentence.
It looks like this comment has not been handled yet.
> ExprCXX.h:120
> + // Check to see if a given overloaded oper
Abpostelnicu updated this revision to Diff 72806.
Abpostelnicu marked an inline comment as done.
Abpostelnicu added a comment.
corrected typo
https://reviews.llvm.org/D22910
Files:
include/clang/AST/ExprCXX.h
lib/AST/Expr.cpp
Index: lib/AST/Expr.cpp
malcolm.parsons added a subscriber: malcolm.parsons.
Comment at: lib/AST/Expr.cpp:2868
@@ +2867,3 @@
+// When looking for potential side-effects, we assume that these
+// operators: assignment, increement and decrement are intended
+// to have a side-effect and other o
Abpostelnicu updated this revision to Diff 72790.
Abpostelnicu marked 2 inline comments as done.
Abpostelnicu added a comment.
i will add the unit tests in the next patch.
https://reviews.llvm.org/D22910
Files:
include/clang/AST/ExprCXX.h
lib/AST/Expr.cpp
Index: lib/AST/Expr.cpp
==
aaron.ballman added a comment.
In https://reviews.llvm.org/D22910#549611, @sylvestre.ledru wrote:
> What about landing this version now (with test) and do the other operators in
> the a second commit? thanks
I don't see a whole lot of value in that, but I may be missing information --
is this
sylvestre.ledru added a comment.
What about landing this version now (with test) and do the other operators in
the a second commit? thanks
https://reviews.llvm.org/D22910
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org
On 21 Sep 2016 6:10 am, "Aaron Ballman" wrote:
aaron.ballman added a comment.
The other thing this patch is missing are tests, btw.
Comment at: lib/AST/Expr.cpp:2869
@@ +2868,3 @@
+// assignment operator is intended to have a side-effect and other
overloaded
+// operat
aaron.ballman added a comment.
The other thing this patch is missing are tests, btw.
Comment at: lib/AST/Expr.cpp:2869
@@ +2868,3 @@
+// assignment operator is intended to have a side-effect and other
overloaded
+// operators are not. Otherwise fall through the logic of
Abpostelnicu updated this revision to Diff 72026.
https://reviews.llvm.org/D22910
Files:
include/clang/AST/ExprCXX.h
lib/AST/Expr.cpp
Index: lib/AST/Expr.cpp
===
--- lib/AST/Expr.cpp
+++ lib/AST/Expr.cpp
@@ -2863,8 +2863,20 @@
Abpostelnicu added inline comments.
Comment at: lib/AST/Expr.cpp:2869
@@ +2868,3 @@
+OverloadedOperatorKind Op = cast(this)->getOperator();
+if (CXXOperatorCallExpr::isAssignmentOp(Op)) {
+ const Decl *FD = cast(this)->getCalleeDecl();
aaron.ballman w
aaron.ballman requested changes to this revision.
This revision now requires changes to proceed.
Comment at: include/clang/AST/ExprCXX.h:109
@@ -108,1 +108,3 @@
+ // Check to see if a given overloaded operator is of assignment kind
+ static bool isAssignmentOp(OverloadedOperat
Abpostelnicu marked 4 inline comments as done.
Abpostelnicu added a comment.
I've added also support for pure in case
CXXOperatorCallExprClass::isAssignmentOperator is true.
https://reviews.llvm.org/D22910
___
cfe-commits mailing list
cfe-commits@l
Abpostelnicu updated this revision to Diff 69205.
https://reviews.llvm.org/D22910
Files:
include/clang/AST/ExprCXX.h
lib/AST/Expr.cpp
Index: lib/AST/Expr.cpp
===
--- lib/AST/Expr.cpp
+++ lib/AST/Expr.cpp
@@ -2861,8 +2861,19 @@
rsmith added inline comments.
Comment at: lib/AST/Expr.cpp:2865-2867
@@ +2864,5 @@
+ case CXXOperatorCallExprClass: {
+// If it is an operator call expr it can have side effects when the
+// underlaying operator is of assignment kind.
+// Othrwise fall through the res
Abpostelnicu marked 2 inline comments as done.
Comment at: lib/AST/Expr.cpp:2868
@@ +2867,3 @@
+OverloadedOperatorKind binOp =
cast(this)->getOperator();
+if (binOp == OO_Equal || (binOp >= OO_PlusEqual && binOp <= OO_PipeEqual))
{
+ return true;
rs
Abpostelnicu removed rL LLVM as the repository for this revision.
Abpostelnicu updated this revision to Diff 68507.
https://reviews.llvm.org/D22910
Files:
include/clang/AST/ExprCXX.h
lib/AST/Expr.cpp
Index: lib/AST/Expr.cpp
===
rsmith added a comment.
If `IncludePossibleEffects` is false, the goal of `HasSideEffect` is to
determine whether we think the user intended for the expression to have a
side-effect. It seems reasonable to assume that any use of an assignment
operator had that intent, so (since it seems like th
Abpostelnicu added a comment.
In https://reviews.llvm.org/D22910#499082, @aaron.ballman wrote:
> Thank you for the patch!
>
> One thing this patch is missing is a test case that exercises this code path.
> For instance, there are diagnostics triggered for expressions with side
> effects when us
aaron.ballman added a comment.
Thank you for the patch!
One thing this patch is missing is a test case that exercises this code path.
For instance, there are diagnostics triggered for expressions with side effects
when used as part of an unevaluated expression like sizeof, noexcept, etc. You
s
I’m trying to get reviews for this patch that modifies Expr::HasSideEffects in
order to have better support for CXXOperatorCallExpr and i don’t know from who
i can ask review.
Thanks
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.
23 matches
Mail list logo
