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

[Tamás Zolnai via Phabricator via cfe-commits]

ztamas marked 4 inline comments as done.
ztamas added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:34-36
+  const auto HasNoSelfCheck = cxxMethodDecl(unless(hasDescendant(
+      binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+                     has(ignoringParenCasts(cxxThisExpr()))))));
----------------
aaron.ballman wrote:
> Will this also match code like:
> ```
> Frobble &Frobble::operator=(const Frobble &F) {
>   if (&F == this->whatever())
>     return *this;
> }
> ```
> or, more insidiously: 
> ```
> Frobble &Frobble::operator=(const Frobble &F) {
>   if (&F == whatever()) // whatever is a member function of Frobble
>     return *this;
> }
> ```
> 
I'll check that case. The original hasLHS(...), hasRHS(...) might work with this use case too.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:54
+      recordType(hasDeclaration(classTemplateSpecializationDecl(
+          hasAnyName("std::shared_ptr", "std::unique_ptr", "std::weak_ptr",
+                     "std::auto_ptr"),
----------------
aaron.ballman wrote:
> These should be using `::std::whatever` to ensure we only care about names in the global namespace named `std`.
> 
> Should this list be configurable? For instance, boost has a fair number of smart pointers.
> Should this list be configurable? For instance, boost has a fair number of smart pointers.

xazax.hun has the same suggestion. I think it's a good idea for a follow-up patch. I actually added a test case with the name CustomPtrField to document this future possibility.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:62-64
+  const auto ThisHasSuspiciousField = cxxMethodDecl(ofClass(cxxRecordDecl(
+      has(fieldDecl(anyOf(hasType(pointerType()), hasType(SmartPointerType),
+                          hasType(arrayType())))))));
----------------
aaron.ballman wrote:
> Hmm, while I understand why you're doing this, I'm worried that it will miss some pretty important cases. For instance, improper thread locking could result in deadlocks, improper releasing of non-memory resources could be problematic (such as network connections, file streams, etc), even simple integer assignments could be problematic in theory:
> ```
> Yobble& Yobble::operator=(const Yobble &Y) {
>   superSecretHashVal = 0; // Being secure!
>   ... some code that may early return ...
>   superSecretHashVal = Y.superSecretHashVal;
> }
> ```
> I wonder whether we want an option here to allow users to diagnose regardless of whether we think it's suspicious or not.
> 
> At the very least, this code should not be enabled for the CERT version of the check as it doesn't conform to the CERT requirements.
It's starting to be too much for me.
First, the problem was false positives. If there are too many false positives then better to have it in the cert module.
Now your problem is that this is not fit with the CERT requirements, because of this matcher which reduces the false positives. Adding this check to the CERT module was not my idea in the first place. So I suggest to have it a simple bugprone check  (and remove the cert alias) and also we can mention that it is related to the corresponding cert rule (but it does not conform with it entirely).
This check allows the usage of copy-and-move pattern, which does not conform with the cert specification either where only the self-check and copy-and-swap is mentioned. So your next suggestion will be to not allow copy-and-move because it does not fit with the cert rule? So I think it's better to have this not a cert check then, but a bugprone check. I prefer to have a working check then implementing a theoretical documentation.

Apart from that cert thing, it actually seems a good idea to add a config option to allow the user to get all catches, not just the "suspicious ones".


================
Comment at: clang-tools-extra/clang-tidy/bugprone/UnhandledSelfAssignmentCheck.cpp:82
+  diag(MatchedDecl->getLocation(),
+       "operator=() might not handle self-assignment properly");
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60507