[PATCH] D16376: clang-tidy check: User-defined copy without assignment

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: clang-tidy/misc/UserDefinedCopyWithoutAssignmentCheck.cpp:37
@@ +36,3 @@
+
+  DiagnosticBuilder Diag = diag(MatchedDecl->getLocation(), "class '%0' defines a copy-constructor but not an assignment operator")
+      << ClassName;
----------------
You should run clang-format over the patch to address 80-col limit issues like this.

================
Comment at: clang-tidy/misc/UserDefinedCopyWithoutAssignmentCheck.cpp:49
@@ +48,3 @@
+  llvm::raw_string_ostream Insertion(S);
+  Insertion << "\n" << ClassName << "& operator = (const " << ClassName << "&) = delete;";
+
----------------
I think we usually prefer `operator=` to `operator =`, but am not certain.

================
Comment at: clang-tidy/misc/UserDefinedCopyWithoutAssignmentCheck.cpp:51
@@ +50,3 @@
+
+  Diag << FixItHint::CreateInsertion(CCtorEnd, Insertion.str());
+}
----------------
We probably do not want to generate this fixit unless we are compiling for at least C++11. We should have a test for C++98.

================
Comment at: clang-tidy/misc/UserDefinedCopyWithoutAssignmentCheck.h:24
@@ +23,3 @@
+/// copy-constructors but no assignement operator and (defensively) defines the
+/// assignment operator to be `= delete`.
+///
----------------
Would it make sense to have an explicitly defaulted copy constructor generate a fixit for an explicitly defaulted copy assignment operator instead of a deleted one? It seems like the user is being explicit about wanting default copy operations more often in that case, and the defaulted copy constructor would not be actively harmful to that.

================
Comment at: docs/clang-tidy/checks/misc-user-defined-copy-without-assignment.rst:24
@@ +23,3 @@
+The fix is defensive. Incorrect compiler-generated assignement can cause
+unexpected behaviour. An explicitly deleted assigneemnt operator will cause a
+compiler error if it is used.
----------------
s/assigneemnt/assignment


Repository:
  rL LLVM

http://reviews.llvm.org/D16376