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