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