[PATCH] D16376: clang-tidy check: Assignment and Construction

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
Comment at: clang-tidy/misc/AssignmentAndConstructionCheck.cpp:91
@@ +90,3 @@
+  case AssignmentAndConstructionCheck::SpecialFunctionKind::CopyConstructor:
+    return "class '%0' defines a copy-constructor but not a copy-assignment "
+           "operator";
----------------
Should elide the single quotes in these diagnostics and pass in the NamedDecl to the diagnostic instead of a string -- the diagnostic formatter handles quoting properly from NamedDecl objects.

Also, the diagnostics should remove the hyphens -- it's a copy constructor, not a copy-constructor (same for assignment , etc).

================
Comment at: clang-tidy/misc/AssignmentAndConstructionCheck.cpp:108
@@ +107,3 @@
+  switch (S) {
+    case AssignmentAndConstructionCheck::SpecialFunctionKind::CopyConstructor:
+    return (llvm::Twine("\n") + ClassName + " &operator=(const " + ClassName +
----------------
This line is not indented properly.

================
Comment at: clang-tidy/misc/AssignmentAndConstructionCheck.cpp:133
@@ +132,3 @@
+  DiagnosticBuilder Diag = diag(MatchedDecl.getLocation(), diagnosticFormat(S))
+                           << ClassName;
+
----------------
You can pass in `&MatchedDecl` instead of a StringRef to get the proper quoting in the diagnostic.

================
Comment at: clang-tidy/misc/AssignmentAndConstructionCheck.cpp:142
@@ +141,3 @@
+
+  std::string FixIt = buildFixIt(MatchedDecl, ClassName, S); (llvm::Twine("\n") + ClassName + " &operator=(const " +
+                    ClassName + " &) = " + deleteOrDefault(MatchedDecl) + ";")
----------------
The extra statement after the call to buildFixIt() looks amiss. ;-)

================
Comment at: clang-tidy/misc/AssignmentAndConstructionCheck.cpp:152
@@ +151,3 @@
+  if (const auto *MatchedDecl =
+          Result.Nodes.getNodeAs<CXXConstructorDecl>("copy-ctor")) {
+    diagnosticAndFixIt(Result, *MatchedDecl,
----------------
Elide braces when the compound body is only one statement (here and elsewhere).

================
Comment at: clang-tidy/misc/AssignmentAndConstructionCheck.cpp:160
@@ +159,3 @@
+  }
+
+  else if (const auto *MatchedDecl =
----------------
Extra newline here.

================
Comment at: clang-tidy/misc/AssignmentAndConstructionCheck.h:31
@@ +30,3 @@
+
+  enum class SpecialFunctionKind {
+    CopyConstructor,
----------------
Since this is an implementation detail, perhaps it can be moved into the source file instead of made a public member of the check?

================
Comment at: clang-tidy/misc/AssignmentAndConstructionCheck.h:39
@@ +38,3 @@
+private:
+  void diagnosticAndFixIt(
+      const ast_matchers::MatchFinder::MatchResult &Result, const CXXMethodDecl &MatchedDecl,
----------------
s/diagnostic/diagnose (to make it a verb)?

================
Comment at: clang-tidy/misc/MiscTidyModule.cpp:93
@@ -91,1 +92,3 @@
+    CheckFactories.registerCheck<AssignmentAndConstructionCheck>(
+        "misc-assignment-and-construction");
     CheckFactories.registerCheck<VirtualNearMissCheck>(
----------------
I don't have a better idea for a name, but this doesn't seem to tell the user much about what the check will do, either.

Would it make sense to expand this check into a rule of (2)/3/(4)/5 check (http://en.cppreference.com/w/cpp/language/rule_of_three) with config options as to which rule to enforce, so that it optionally includes the destructor? I'm thinking of config options like: RuleOf=3|5, IncludeDestructors=true|false with the defaults being 3 and true, respectively? The check could be named misc-cpp-special-member-rule (or something more clear than that). As it stands, the check currently handles the rule of 2 and 4, but I think a lot of people may want the rule of 3 or 5 instead and it would be good to cover them all under the same check name.

Note, this could be done in a follow-up patch, I am mostly interested in getting the name correct for the check.

================
Comment at: docs/clang-tidy/checks/misc-assignment-and-construction.rst:8
@@ +7,3 @@
+constructor.  This behaviour is deprecated by the standard (C++ 14 draft
+standard [class.construction paragraph 18]).
+
----------------
No need to mention draft standard; C++14 is an IS. Also, the citation should be: [class.copy] paragraph 18.

================
Comment at: docs/clang-tidy/checks/misc-assignment-and-construction.rst:18
@@ +17,3 @@
+This check finds classes where only one of the copy/move constructor and
+copy/move assignment operator is user-defined.  The fix is defensive. Incorrect
+compiler-generated assignment/construction can cause unexpected behaviour. An
----------------
May want to expound on what you mean by the fix being defensive.

================
Comment at: test/clang-tidy/misc-assignment-and-construction.cpp:161
@@ +160,2 @@
+  E4 &operator=(E4 &&);
+};
----------------
Can we also have a test that shows a deleted operation generates a deleted companion? e.g.,
```
struct S {
  S(const S&) = delete;
  // check that it adds S& operator=(const S&) = delete;
};
```


Repository:
  rL LLVM

http://reviews.llvm.org/D16376