Re: [PATCH] Add clang-tidy check for unique_ptr's reset+release→move

[Alexander Kornienko]

Thank you for the check!

Looks generally fine, but needs some polishing to match LLVM style.

================
Comment at: clang-tidy/misc/UniqueptrResetRelease.cpp:14
@@ +13,3 @@
+
+// #include "clang/AST/ASTContext.h"
+// #include "clang/AST/Decl.h"
----------------
Please remove commented lines.

================
Comment at: clang-tidy/misc/UniqueptrResetRelease.cpp:39
@@ +38,3 @@
+void UniqueptrResetRelease::check(
+    const ast_matchers::MatchFinder::MatchResult &result) {
+  const auto* reset_member = result.Nodes.getNodeAs<MemberExpr>("reset_member");
----------------
You're already using ast_matchers, so you can avoid it here.

================
Comment at: clang-tidy/misc/UniqueptrResetRelease.cpp:40
@@ +39,3 @@
+    const ast_matchers::MatchFinder::MatchResult &result) {
+  const auto* reset_member = result.Nodes.getNodeAs<MemberExpr>("reset_member");
+  const auto* release_member =
----------------
Please use CamelCase for local variables.

================
Comment at: clang-tidy/misc/UniqueptrResetRelease.cpp:55
@@ +54,3 @@
+
+  if (reset_member->isArrow()) {
+    left_text = "*" + left_text;
----------------
In LLVM style single-statement if/for/... bodies don't need braces (and should not be on the same line with the if/for/...).

================
Comment at: clang-tidy/misc/UniqueptrResetRelease.cpp:68
@@ +67,3 @@
+  diag(reset_member->getExprLoc(),
+       "Prefer std::move over ptr1.reset(ptr2.release())")
+      << FixItHint::CreateReplacement(
----------------
Clang warnings don't start with a capital letter. We try to move to this style in clang-tidy as well.

================
Comment at: clang-tidy/misc/UniqueptrResetRelease.cpp:68
@@ +67,3 @@
+  diag(reset_member->getExprLoc(),
+       "Prefer std::move over ptr1.reset(ptr2.release())")
+      << FixItHint::CreateReplacement(
----------------
alexfh wrote:
> Clang warnings don't start with a capital letter. We try to move to this style in clang-tidy as well.
Maybe "prefer ptr1 = std::move(ptr2); ..."? It would be more wordy, but also clearer, I think.

================
Comment at: clang-tidy/misc/UniqueptrResetRelease.cpp:74
@@ +73,3 @@
+
+}  // namespace tidy
+}  // namespace clang
----------------
LLVM style: one space before comments, please.

================
Comment at: clang-tidy/misc/UniqueptrResetRelease.h:28
@@ +27,3 @@
+public:
+  using ClangTidyCheck::ClangTidyCheck;
+  void registerMatchers(ast_matchers::MatchFinder *finder) override;
----------------
Unfortunately, delegated constructors are not implemented in some of the compilers supported by Clang (VS2012). You need to manually delegate the constructor.

================
Comment at: clang-tidy/misc/UniqueptrResetRelease.h:29
@@ +28,3 @@
+  using ClangTidyCheck::ClangTidyCheck;
+  void registerMatchers(ast_matchers::MatchFinder *finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &result) override;
----------------
Here and below: in LLVM style names of local variables and function parameters should start with a capital letter.

================
Comment at: test/clang-tidy/uniqueptr-reset-release.cpp:4
@@ +3,3 @@
+
+// CHECK-NOT: warning
+
----------------
check_clang_tidy.sh calls FileCheck with -implicit-check-not='warning:|error:', so this is superfluous.

================
Comment at: test/clang-tidy/uniqueptr-reset-release.cpp:35
@@ +34,3 @@
+  a.reset(c.release());
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Prefer std::move over ptr1.reset(ptr2.release()) [misc-uniqueptr-reset-release]
+  // CHECK-FIXES: a = std::move(c);
----------------
You can omit the " [misc-uniqueptr-reset-release]" part in all the messages except for the first one. It will make the test slightly more readable.

http://reviews.llvm.org/D6485