Re: [PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

2018-03-26 Thread David Blaikie via cfe-commits
Historically Clang's policy on warnings was, I think, much more conservative than it seems to be today. There was a strong desire not to implement off-by-default warnings, and to have warnings with an exceptionally low false-positive rate - maybe the user-defined operator detection was either assum

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

2018-03-26 Thread David Blaikie via Phabricator via cfe-commits
dblaikie added a subscriber: lebedev.ri. dblaikie added a comment. Historically Clang's policy on warnings was, I think, much more conservative than it seems to be today. There was a strong desire not to implement off-by-default warnings, and to have warnings with an exceptionally low false-positi

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

2018-03-26 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added a comment. In https://reviews.llvm.org/D44883#1048689, @rjmccall wrote: > I tracked Chandler down, and he doesn't remember why user-defined operators > are excluded. Ok then. Unless reviewers think it would be best to have separate diag groups for "builtin" and "user-provided

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

2018-03-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I tracked Chandler down, and he doesn't remember why user-defined operators are excluded. Repository: rC Clang https://reviews.llvm.org/D44883 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.or

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

2018-03-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Is D not considered trivial there? Ugh, C++. Well, we certainly could justify treating D as a builtin operator as well, since it doesn't involve running any user code, but I don't think you're obligated to write a whole bunch of analysis just to make that work if the

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

2018-03-26 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added a comment. Ok, once we understand why "field" diag is separate, and why since initial implementation it only warned for "builtin" operators, i'll work on the code further. In https://reviews.llvm.org/D44883#1048485, @rjmccall wrote: > That's an interesting question! You're ab

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

2018-03-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D44883#1048485, @rjmccall wrote: > In https://reviews.llvm.org/D44883#1048439, @lebedev.ri wrote: > > > In https://reviews.llvm.org/D44883#1048400, @rjmccall wrote: > > > > > Yeah, like I said, I'm just worried about the usability of having >

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

2018-03-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D44883#1048439, @lebedev.ri wrote: > In https://reviews.llvm.org/D44883#1048421, @Quuxplusone wrote: > > > Two more high-level comments from me, and then I'll try to butt out. > > > > Q1. I don't understand what `field-` is doing in the name o

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

2018-03-26 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added a subscriber: thakis. lebedev.ri added a comment. In https://reviews.llvm.org/D44883#1048421, @Quuxplusone wrote: > Two more high-level comments from me, and then I'll try to butt out. > > Q1. I don't understand what `field-` is doing in the name of some > diagnostics. Is there

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

2018-03-26 Thread Arthur O'Dwyer via Phabricator via cfe-commits
Quuxplusone added a comment. Two more high-level comments from me, and then I'll try to butt out. Q1. I don't understand what `field-` is doing in the name of some diagnostics. Is there a situation where `x = x;` is "less/more of an error" just because `x` is a {data member, lambda capture, str

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

2018-03-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D44883#1048081, @lebedev.ri wrote: > In https://reviews.llvm.org/D44883#1048010, @rjmccall wrote: > > > I'm not sure you really need to put these in their own warning sub-group > > just because they're user-defined operators. That's especial

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

2018-03-26 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri updated this revision to Diff 139788. lebedev.ri marked 3 inline comments as done. lebedev.ri added a comment. - Also warn on `BO_DivAssign`, `BO_SubAssign`, `BO_XorAssign`. - Add releasenotes entry. Repository: rC Clang https://reviews.llvm.org/D44883 Files: docs/ReleaseNotes.r

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

2018-03-26 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added a comment. In https://reviews.llvm.org/D44883#1048010, @rjmccall wrote: > I'm not sure you really need to put these in their own warning sub-group just > because they're user-defined operators. That's especially true because it > appears we already have divisions in the warnin

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

2018-03-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I'm not sure you really need to put these in their own warning sub-group just because they're user-defined operators. That's especially true because it appears we already have divisions in the warning group based on the form of the l-value; we don't want this to go co

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

2018-03-25 Thread Arthur O'Dwyer via Phabricator via cfe-commits
Quuxplusone added inline comments. Comment at: lib/Sema/SemaExpr.cpp:12087 + case BO_AndAssign: + case BO_OrAssign: +DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, /*IsBuiltin=*/false); I understand why `x &= x` and `x |= x` are mathematically special for the

[PATCH] D44883: [Sema] Extend -Wself-assign with -Wself-assign-overloaded to warn on overloaded self-assignment (classes)

2018-03-25 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri created this revision. lebedev.ri added reviewers: aaron.ballman, rsmith, rtrieu, nikola, chandlerc, rjmccall. This has just bit me, so i though it would be nice to avoid that next time :) Motivational case: https://godbolt.org/g/cq9UNk Basically, it's likely to happen if you don't