Add clang-tidy check for unique_ptr's reset+release→move

[sokolov]

Summary

Replace x.reset(y.release()); with x = std::move(y);
If y is rvalue, replace with x = y; instead

[alexfh]

Thank you for the check!

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

clang-tidy/misc/UniqueptrResetRelease.cpp

14

Please remove commented lines.

39

You're already using ast_matchers, so you can avoid it here.

40

Please use CamelCase for local variables.

55

In LLVM style single-statement if/for/... bodies don't need braces (and should not be on the same line with the if/for/...).

68

Clang warnings don't start with a capital letter. We try to move to this style in clang-tidy as well.

68

Maybe "prefer ptr1 = std::move(ptr2); ..."? It would be more wordy, but also clearer, I think.

74

LLVM style: one space before comments, please.

clang-tidy/misc/UniqueptrResetRelease.h

28

Unfortunately, delegated constructors are not implemented in some of the compilers supported by Clang (VS2012). You need to manually delegate the constructor.

29

Here and below: in LLVM style names of local variables and function parameters should start with a capital letter.

test/clang-tidy/uniqueptr-reset-release.cpp

4

check_clang_tidy.sh calls FileCheck with -implicit-check-not='warning:|error:', so this is superfluous.

35

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.

[sokolov]

Done

[alexfh]

Awesome, thanks!

[alexfh]

Do you have commit rights, BTW? If not, I can commit this patch for you.

[sokolov]

I don't have commit rights, I didn't submit that many patches so far :)
Please commit.

[alexfh]

Committed as r223460 with a few minor changes:
Renamed the test file to be closer to the check name, re-formatted the test to LLVM style and made the CHECK-MESSAGES lines shorter.

[alexfh]

test/clang-tidy/uniqueptr-reset-release.cpp

37

The message for this case shouldn't mention std::move. Can you fix this?

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   BoolPointerImplicitConversion.cpp
   MiscTidyModule.cpp
   SwappedArgumentsCheck.cpp
   UndelegatedConstructor.cpp
   UnusedRAII.cpp
+  UniqueptrResetRelease.cpp
   UseOverride.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   )

clang-tidy/misc/MiscTidyModule.cpp

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
 #include "BoolPointerImplicitConversion.h"
 #include "SwappedArgumentsCheck.h"
 #include "UndelegatedConstructor.h"
 #include "UnusedRAII.h"
+#include "UniqueptrResetRelease.h"
 #include "UseOverride.h"
 
 namespace clang {
 namespace tidy {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ArgumentCommentCheck>("misc-argument-comment");
     CheckFactories.registerCheck<BoolPointerImplicitConversion>(
         "misc-bool-pointer-implicit-conversion");
     CheckFactories.registerCheck<SwappedArgumentsCheck>(
         "misc-swapped-arguments");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "misc-undelegated-constructor");
+    CheckFactories.registerCheck<UniqueptrResetRelease>(
+        "misc-uniqueptr-reset-release");
     CheckFactories.registerCheck<UnusedRAIICheck>("misc-unused-raii");
     CheckFactories.registerCheck<UseOverride>("misc-use-override");
   }
 };
 
 // Register the MiscTidyModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<MiscModule>
 X("misc-module", "Adds miscellaneous lint checks.");
 
 // This anchor is used to force the linker to link in the generated object file
 // and thus register the MiscModule.
 volatile int MiscModuleAnchorSource = 0;
 
 } // namespace tidy
 } // namespace clang

clang-tidy/misc/UniqueptrResetRelease.h

+//===--- UniqueptrResetRelease.h - clang-tidy -------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNIQUEPTR_RESET_RELEASE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNIQUEPTR_RESET_RELEASE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// \brief Find and replace unique_ptr::reset(release()) with std::move
+///
+/// Example:
+///   std::unique_ptr<Foo> x, y;
+///   x.reset(y.release()); -> x = std::move(y);
+///
+/// If "y" is already rvalue, std::move is not added.
+/// "x" and "y" can also be std::unique_ptr<Foo>*.
+class UniqueptrResetRelease : public ClangTidyCheck {
+public:
+  using ClangTidyCheck::ClangTidyCheck;
+  void registerMatchers(ast_matchers::MatchFinder *finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_UNIQUEPTR_RESET_RELEASE_H

clang-tidy/misc/UniqueptrResetRelease.cpp

+//===--- UniqueptrResetRelease.cpp - clang-tidy ---------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "UniqueptrResetRelease.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+// #include "clang/AST/ASTContext.h"
+// #include "clang/AST/Decl.h"
+// #include "clang/Basic/CharInfo.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void UniqueptrResetRelease::registerMatchers(MatchFinder *finder) {
+  finder->addMatcher(
+      memberCallExpr(
+          on(expr().bind("left")), callee(memberExpr().bind("reset_member")),
+          callee(methodDecl(hasName("reset"),
+                            ofClass(hasName("::std::unique_ptr")))),
+          has(memberCallExpr(
+              on(expr().bind("right")),
+              callee(memberExpr().bind("release_member")),
+              callee(methodDecl(hasName("release"),
+                                ofClass(hasName("::std::unique_ptr")))))))
+          .bind("reset_call"),
+      this);
+}
+
+void UniqueptrResetRelease::check(
+    const ast_matchers::MatchFinder::MatchResult &result) {
+  const auto* reset_member = result.Nodes.getNodeAs<MemberExpr>("reset_member");
+  const auto* release_member =
+      result.Nodes.getNodeAs<MemberExpr>("release_member");
+  const auto* right = result.Nodes.getNodeAs<Expr>("right");
+  const auto* left = result.Nodes.getNodeAs<Expr>("left");
+  const auto* reset_call =
+      result.Nodes.getNodeAs<CXXMemberCallExpr>("reset_call");
+
+  std::string left_text = clang::Lexer::getSourceText(
+      CharSourceRange::getTokenRange(left->getSourceRange()),
+      *result.SourceManager, result.Context->getLangOpts());
+  std::string right_text = clang::Lexer::getSourceText(
+      CharSourceRange::getTokenRange(right->getSourceRange()),
+      *result.SourceManager, result.Context->getLangOpts());
+
+  if (reset_member->isArrow()) {
+    left_text = "*" + left_text;
+  }
+  if (release_member->isArrow()) {
+    right_text = "*" + right_text;
+  }
+  // Even if x was rvalue, *x is not rvalue anymore
+  if (!right->isRValue() || release_member->isArrow()) {
+    right_text = "std::move(" + right_text + ")";
+  }
+  std::string new_text = left_text + " = " + right_text;
+
+  diag(reset_member->getExprLoc(),
+       "Prefer std::move over ptr1.reset(ptr2.release())")
+      << FixItHint::CreateReplacement(
+          CharSourceRange::getTokenRange(reset_call->getSourceRange()),
+          new_text);
+}
+
+}  // namespace tidy
+}  // namespace clang

test/clang-tidy/uniqueptr-reset-release.cpp

+// RUN: $(dirname %s)/check_clang_tidy.sh %s misc-uniqueptr-reset-release %t
+// REQUIRES: shell
+
+// CHECK-NOT: warning
+
+namespace std {
+template <typename T>
+struct unique_ptr {
+  unique_ptr<T>();
+  explicit unique_ptr<T>(T*);
+  template <typename U>
+  unique_ptr<T>(unique_ptr<U>&&);
+  void reset(T*);
+  T* release();
+};
+}  // namespace std
+
+struct Foo {};
+struct Bar : Foo {};
+
+std::unique_ptr<Foo> Create();
+std::unique_ptr<Foo>& Look();
+std::unique_ptr<Foo>* Get();
+
+void f() {
+  std::unique_ptr<Foo> a, b;
+  std::unique_ptr<Bar> c;
+  std::unique_ptr<Foo>* x = &a;
+  std::unique_ptr<Foo>* y = &b;
+
+  a.reset(b.release());
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Prefer std::move over ptr1.reset(ptr2.release()) [misc-uniqueptr-reset-release]
+  // CHECK-FIXES: a = std::move(b);
+  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);
+  a.reset(Create().release());
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Prefer std::move over ptr1.reset(ptr2.release()) [misc-uniqueptr-reset-release]
+  // CHECK-FIXES: a = Create();
+  x->reset(y->release());
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: Prefer std::move over ptr1.reset(ptr2.release()) [misc-uniqueptr-reset-release]
+  // CHECK-FIXES: *x = std::move(*y);
+  Look().reset(Look().release());
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Prefer std::move over ptr1.reset(ptr2.release()) [misc-uniqueptr-reset-release]
+  // CHECK-FIXES: Look() = std::move(Look());
+  Get()->reset(Get()->release());
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: Prefer std::move over ptr1.reset(ptr2.release()) [misc-uniqueptr-reset-release]
+  // CHECK-FIXES: *Get() = std::move(*Get());
+}
+