Page MenuHomePhabricator

Add support for CXXOperatorCallExpr in Expr::HasSideEffects
Needs ReviewPublic

Authored by Abpostelnicu on Jul 28 2016, 3:39 AM.

Details

Summary

As the there is support for BinaryOperator in order to see that it's of assignment type, this patch adds the same logic for the cases when assignment operators have been overloaded and used in different expressions.

For example:

class Number {
  int number;
public:
  Number& operator=(int a) {
    number = a;
    return *this;
  }
};

int main(int argc, const char * argv[]) {
  Number a;
  a = 2;
}

The AST node that will get generated for the assignment is of type CXXOperatorCallExpr so calling HasSideEffects on that expression would have resulted in a false return since CXXOperatorCallExpr was not considered to have a possible side effect when it's underlying operator type is assignment.

This patch addresses this exact issue and checks the underlying kind and if it's assignment it returns true, otherwise it continues the execution.

Diff Detail

Event Timeline

Abpostelnicu retitled this revision from to Add support for CXXOperatorCallExpr in Expr::HasSideEffects.
Abpostelnicu updated this object.
Abpostelnicu set the repository for this revision to rL LLVM.
Abpostelnicu added projects: Restricted Project, Restricted Project.
aaron.ballman removed a project: Restricted Project.
aaron.ballman added a subscriber: cfe-commits.
aaron.ballman edited edge metadata.Jul 28 2016, 5:00 AM

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 should include a test case that demonstrates some behavioral change in the compiler that this patch addresses.

The AST node that will get generated for the assignment is of type CXXOperatorCallExpr so calling HasSideEffects on that expression would have resulted in a false return since CXXOperatorCallExpr was not considered to have a possible side effect when it's underlying operator type is assignment.

I think the underlying issue here is that HasSideEffects() accepts an argument as to whether possible side effects should count as definite side effects, and for the unevaluated context diagnostics, we do not want to include possible side effects, only definite ones. For instance, consider this very common code pattern where the function definition is not available to the TU:

struct S {
  S& operator=(int);
};

There's no way to know whether side effects will or won't happen for an assignment operator on an object of the above type because the definition does not exist. Assuming that side effects *definitely* happen, as your patch does, can then trigger false positives. So the second question is: how many false-positives will it generate? I think it may be reasonable to assume that operator=() has side effects, but you should run some large C++ projects (like LLVM itself) through Clang to see how many new diagnostics this change triggers and how many of those diagnostics are true positives vs false positives. That will tell us for sure whether this is adding value or not.

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 should include a test case that demonstrates some behavioral change in the compiler that this patch addresses.

The AST node that will get generated for the assignment is of type CXXOperatorCallExpr so calling HasSideEffects on that expression would have resulted in a false return since CXXOperatorCallExpr was not considered to have a possible side effect when it's underlying operator type is assignment.

I think the underlying issue here is that HasSideEffects() accepts an argument as to whether possible side effects should count as definite side effects, and for the unevaluated context diagnostics, we do not want to include possible side effects, only definite ones. For instance, consider this very common code pattern where the function definition is not available to the TU:

struct S {
  S& operator=(int);
};

There's no way to know whether side effects will or won't happen for an assignment operator on an object of the above type because the definition does not exist. Assuming that side effects *definitely* happen, as your patch does, can then trigger false positives. So the second question is: how many false-positives will it generate? I think it may be reasonable to assume that operator=() has side effects, but you should run some large C++ projects (like LLVM itself) through Clang to see how many new diagnostics this change triggers and how many of those diagnostics are true positives vs false positives. That will tell us for sure whether this is adding value or not.

You are right, using this patch i've built the firefox codebase, witch is rather a very large product, and there is no issues triggered. I will also add some test cases.

rsmith edited edge metadata.Aug 10 2016, 2:11 PM

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 this is not adding false positives) this seems reasonable to me.

lib/AST/Expr.cpp
2864–2880

Please add a comment indicating the fallthrough here is intentional.

2867–2868

Variable names should start with a capital letter.

2868

Please don't hard-code the order of OO enumerators like this (and this isn't even correct: you missed <<= and >>=).

Instead, consider extending BinaryOperator::isAssignmentOp / BinaryOperator::getOverloadedOpcode so you can use them for this.

Abpostelnicu marked 2 inline comments as done.Aug 18 2016, 4:06 AM
Abpostelnicu added inline comments.
lib/AST/Expr.cpp
2868

i was thinking more on doing for this specific case since BinaryOperator::isAssignmentOp and BinaryOperator::getOverloadedOpcode only accepts binary op codes and CXXOperatorCallExpr incapsulates operators both binary and unary

Abpostelnicu edited edge metadata.
Abpostelnicu removed rL LLVM as the repository for this revision.
rsmith added inline comments.Aug 18 2016, 12:25 PM
lib/AST/Expr.cpp
2865–2867

Well, this is true for any overloaded operator. Perhaps something like

"When looking for potential side-effects, we assume that an overloaded assignment operator is intended to have a side-effect and other overloaded operators are not."

2869–2871

I think this should go after the check for a pure callee -- if for whatever reason someone has defined an operator= with __attribute__((pure)), we shouldn't claim it has a side-effect.

Abpostelnicu marked 4 inline comments as done.

I've added also support for pure in case CXXOperatorCallExprClass::isAssignmentOperator is true.

aaron.ballman requested changes to this revision.Aug 29 2016, 5:35 AM
aaron.ballman edited edge metadata.
aaron.ballman added inline comments.
include/clang/AST/ExprCXX.h
109 ↗(On Diff #69205)

Missing the full stop at the end of the sentence.

lib/AST/Expr.cpp
2864–2881

I think this is still missing the comment about fallthrough. Please use LLVM_FALLTHROUGH.

2869

Should this also handle overloaded ++ and -- if they're the prefix forms? What about overloaded operator new and delete (and the array forms)?

This revision now requires changes to proceed.Aug 29 2016, 5:35 AM
Abpostelnicu added inline comments.Sep 3 2016, 2:20 AM
lib/AST/Expr.cpp
2869

I think for now i should prefer only to have the implementation for assignment operator when it's overloaded.

Abpostelnicu edited edge metadata.

The other thing this patch is missing are tests, btw.

lib/AST/Expr.cpp
2869

I think that operator++() and operator--() should be handled as well as the assignment operators. @rsmith, thoughts?

What about landing this version now (with test) and do the other operators in the a second commit? thanks

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 patch gating some other work? Supporting increment and decrement should be pretty straight-forward and is logically related to the current changeset. I'd prefer to fix it all at once since it shouldn't be overly onerous (and it protects us from accidentally only implementing part of the functionality), but if there's a reason to split it out, then I don't see harm in that either.

Abpostelnicu edited edge metadata.
Abpostelnicu marked 2 inline comments as done.

i will add the unit tests in the next patch.

malcolm.parsons added inline comments.
lib/AST/Expr.cpp
2868

typo: increment

Abpostelnicu marked an inline comment as done.

corrected typo

This patch is missing tests for the new functionality.

include/clang/AST/ExprCXX.h
109 ↗(On Diff #69205)

It looks like this comment has not been handled yet.

120 ↗(On Diff #72806)

I'm not certain that isIncrementOp or isDecrementOp are useful predicate functions. I would test these conditions inside of Expr.cpp directly.

Abpostelnicu marked 3 inline comments as done.

Can someone please show me an example on how to write a test for this patch?

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_unevaluated_context diagnostics), such as use as the operand to sizeof.

aaron.ballman resigned from this revision.Mar 2 2018, 5:56 AM