[PATCH] D60507: [clang-tidy] new check: bugprone-unhandled-self-assignment

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D60507#1491095 <https://reviews.llvm.org/D60507#1491095>, @ztamas wrote:

> Ping.
>  Is it good to go or is there anything else I need to include in this patch?


Sorry for the delay, I was gone for WG14 meetings last week, which made reviewing more involved patches somewhat difficult. I'm back now and starting to dig out from under the mountain of emails.

> Among the lots of idea, I'm not sure which is just brainstorming and which is a change request.

Generally speaking, all feedback is a bit of a mixture of the two. They're change requests, but we can definitely brainstorm whether the request makes sense, how to do it, whether there are better changes to make instead, etc. We usually say something explicit like "but I don't insist" when we're just spitballing ideas. That said, I'm sorry if I was unclear on what changes I was insisting on.

I definitely want to see the diagnostic text changed away from "might be" -- that's too noncommittal for my tastes. I'd also like to see the additional test case (I suspect it will just work out of the box, but if it doesn't, it would be good to know about it). The CERT alias is a bit more of a grey area -- I'd like to insist on it being added because it's trivial, it improves the behavior of this check by introducing an option some users will want, and checks conforming to coding standards are always more compelling than one-off checks. However, you seem very resistant to that and I don't want to add to your work load if you are opposed to doing it, so I don't insist on the change.

> The check seems to work (has useful catches and does not produce too many false positives), however, it does not conform with the corresponding cert rule. I expect that a cert alias with an option can be added in a follow-up patch (option to ignore ThisHasSuspiciousField pattern). The bugprone-* check would have the same default behavior as it has now in this patch. So adding the cert alias would not change this behavior, right? In this case, this code change can be added in a separate patch I guess.

Agreed.



================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:82
+  diag(MatchedDecl->getLocation(),
+       "operator=() might not handle self-assignment properly");
+}
----------------
ztamas wrote:
> aaron.ballman wrote:
> > ztamas wrote:
> > > aaron.ballman wrote:
> > > > Hmm, the "might not" seems a bit flat to me. How about: `'operator=()' does not properly test for self-assignment`?
> > > > 
> > > > Also, do we want to have a fix-it to insert a check for self assignment at the start of the function?
> > > I don't think "test for self-assignment" will be good, because it's only one way to make the operator self assignment safe. In case of copy-and-swap and copy-and-move there is no testing of self assignment, but swaping/moving works in all cases without explicit self check.
> > > 
> > > A fix-it can be a good idea for a follow-up patch.
> > Good point on the use of "test" in the diagnostic. My concern is more with it sounding like we don't actually know -- the "might" is what's bothering me. I think if we switch it to "does not", it'd be a bit better.
> We don't actually know. We check only some patterns, but there might be other operator=() implementations which are self assignment safe (see false positive test cases).
I understand that we're using a heuristic, but that doesn't mean we use language like "might not" in the resulting diagnostic. Either we think the code is correct (we don't diagnose) or the code doesn't do what it needs to do to pass our check (we diagnose). That's why I recommended "does not" -- the operator=() does not handle self-assignment properly according to your check. I couldn't find any existing diagnostics saying the code "might not" do something properly.


================
Comment at: clang-tools-extra/test/clang-tidy/bugprone-unhandled-self-assignment.cpp:214
+};
+
+///////////////////////////////////////////////////////////////////
----------------
Does the check diagnose code like this?
```
template <typename Ty, typename Uy>
class C {
  Ty *Ptr1;
  Uy *Ptr2;

public:
  C& operator=(const C<Uy, Ty> &RHS) {
    Ptr1 = RHS.getUy();
    Ptr2 = RHS.getTy();
    return *this;
  }

  Ty *getTy() const { return Ptr1; }
  Uy *getUy() const { return Ptr2; }
};
```
I'm hoping we don't consider it a copy assignment operator and thus don't diagnose (note that the template argument order is reversed in the assignment operator parameter type).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60507/new/

https://reviews.llvm.org/D60507