[PATCH] D22910: Add support for CXXOperatorCallExpr in Expr::HasSideEffects

[Aaron Ballman via cfe-commits]

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 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.


Repository:
  rL LLVM

https://reviews.llvm.org/D22910