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