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