Create modernize-make-unique check.

[angelgarcia]

Summary

create a check that replaces 'std::unique_ptr<type>(new type(args...))' with 'std::make_unique<type>(args...)'. It was on the list of "Ideas for new Tools". It needs to be tested more carefully, but first I wanted to know if you think it is worth the effort.

[klimek]

This is definitely a useful check to have in modernize.

clang-tidy/modernize/MakeUniqueCheck.cpp

26–28

How can that ever return 0 if you have matchesName("::std::unique_ptr") below?

52

Why do we need the hasCanonicalType?

[angelgarcia]

Remove 'hasCanonicalType'.

clang-tidy/modernize/MakeUniqueCheck.cpp

26–28

Maybe using macros or something like that. As I said, it needs to be more carefully tested.

[klimek]

clang-tidy/modernize/MakeUniqueCheck.cpp

26–28

That should not be possible for the spelling location. Generally, I'd avoid writing code that doesn't fix a breaking test.

[angelgarcia]

Two tests in which 'getTokenLength' returns 0.

[klimek]

In D13166#254520, @angelgarcia wrote:

Two tests in which 'getTokenLength' returns 0.

Thanks for the tests - question is: I would have expected us to use something like Lexer::getSourceText, which should give us the full range in the first test (do we not want to replace it?) and tell us that we don't have the type in one consecutive range in the second case.

[angelgarcia]

How can Lexer::getSourceText give us the range? My problem precisely is
that I couldn't find any way to obtain the range of the constructor call
without the template arguments.

[angelgarcia]

I think this is a bit better than before.

[klimek]

clang-tidy/modernize/MakeUniqueCheck.cpp

76–77

Comment what cases this can happen in, and why we don't want to replace in those cases...

[angelgarcia]

This raises a question. Do we want to do replacements when we use an alias
for std::unique_ptr? That fact that something is an unique_ptr might be an
implementation detail that should not be exposed, but it could also happen
that the alias is there only for brevity. What do you think?

[klimek]

In D13166#254730, @angelgarcia wrote:

This raises a question. Do we want to do replacements when we use an alias
for std::unique_ptr? That fact that something is an unique_ptr might be an
implementation detail that should not be exposed, but it could also happen
that the alias is there only for brevity. What do you think?

Good question. Generally, it seems to me that aliasing unique_ptr wouldn't seem useful apart from having a shortcut; unique_ptr is special in a couple of ways, which would make it particularly weird as an "implementation detail".

My take on it is that we want to replace those, until we find real use cases that warrant a more complex implementation.

[angelgarcia]

Also replace on aliases.

[klimek]

apart from the comment, LG

clang-tidy/modernize/MakeUniqueCheck.cpp

68–71

Please add a comment what this does (in which cases do we want to insert the type).

[angelgarcia]

Add a comment.

clang-tidy/modernize/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyModernizeModule
   LoopConvertCheck.cpp
   LoopConvertUtils.cpp
+  MakeUniqueCheck.cpp
   ModernizeTidyModule.cpp
   PassByValueCheck.cpp
   ReplaceAutoPtrCheck.cpp
   ShrinkToFitCheck.cpp
   UseAutoCheck.cpp
   UseNullptrCheck.cpp
   UseOverrideCheck.cpp
 

clang-tidy/modernize/MakeUniqueCheck.h

+//===--- MakeUniqueCheck.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_MODERNIZE_MAKE_UNIQUE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_MAKE_UNIQUE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// Replace the pattern:
+/// \code
+///   std::unique_ptr<type>(new type(args...))
+/// \endcode
+///
+/// With the C++14 version:
+/// \code
+///   std::make_unique<type>(args...)
+/// \endcode
+class MakeUniqueCheck : public ClangTidyCheck {
+public:
+  MakeUniqueCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_MAKE_UNIQUE_H
+

clang-tidy/modernize/MakeUniqueCheck.cpp

+//===--- MakeUniqueCheck.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 "MakeUniqueCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+const char PointerType[] = "pointerType";
+const char ConstructorCall[] = "constructorCall";
+const char NewExpression[] = "newExpression";
+
+/// \brief Returns the length of the token that goes since the beggining of the
+/// constructor call until the '<' of the template. This token should either be
+/// 'unique_ptr' or 'std::unique_ptr'. In any other case, it returns 0.
+static int getTokenLength(SourceLocation TokenStart, const SourceManager &SM,
+                          const LangOptions &LO) {
+  SmallVector<char, 10> Buffer;
+  bool Invalid = false;
+  StringRef Token = Lexer::getSpelling(TokenStart, Buffer, SM, LO, &Invalid);
+
+  if (!Invalid && Token == "unique_ptr")
+    return Token.size();
+
+  if (!Invalid && Token == "std") {
+    Token = Lexer::getSpelling(TokenStart.getLocWithOffset(5), Buffer, SM, LO,
+                               &Invalid);
+    if (!Invalid && Token == "unique_ptr")
+      return strlen("std::unique_ptr");
+  }
+  return 0;
+}
+
+void MakeUniqueCheck::registerMatchers(MatchFinder *Finder) {
+  if (getLangOpts().CPlusPlus11) {
+    Finder->addMatcher(
+        cxxBindTemporaryExpr(has(
+            cxxConstructExpr(
+                hasType(qualType(hasDeclaration(classTemplateSpecializationDecl(
+                    matchesName("::std::unique_ptr"),
+                    templateArgumentCountIs(1),
+                    hasTemplateArgument(
+                        0, templateArgument(
+                               refersToType(qualType().bind(PointerType)))))))),
+                argumentCountIs(1),
+                hasArgument(0, cxxNewExpr(hasType(pointsTo(qualType(
+                                              equalsBoundNode(PointerType)))))
+                                   .bind(NewExpression)))
+                .bind(ConstructorCall))),
+        this);
+  }
+}
+
+void MakeUniqueCheck::check(const MatchFinder::MatchResult &Result) {
+  SourceManager &SM = *Result.SourceManager;
+  const auto *Construct =
+      Result.Nodes.getNodeAs<CXXConstructExpr>(ConstructorCall);
+  const auto *New = Result.Nodes.getNodeAs<CXXNewExpr>(NewExpression);
+
+  SourceLocation ConstructCallStart = Construct->getExprLoc();
+  int Len = getTokenLength(ConstructCallStart, SM, LangOptions());
+  if (Len == 0)
+    return;
+  SourceLocation ConstructCallEnd =
+      ConstructCallStart.getLocWithOffset(Len - 1);
+
+  auto Diag = diag(ConstructCallStart, "use std::make_unique instead")
+              << FixItHint::CreateReplacement(
+                  SourceRange(ConstructCallStart, ConstructCallEnd),
+                  "std::make_unique");
+
+  SourceLocation NewStart = New->getSourceRange().getBegin();
+  SourceLocation NewEnd = New->getSourceRange().getEnd();
+  switch (New->getInitializationStyle()) {
+  case CXXNewExpr::NoInit: {
+
+    Diag << FixItHint::CreateRemoval(SourceRange(NewStart, NewEnd));
+    break;
+  }
+  case CXXNewExpr::CallInit: {
+    SourceRange InitRange = New->getDirectInitRange();
+    Diag << FixItHint::CreateRemoval(
+        SourceRange(NewStart, InitRange.getBegin()));
+    Diag << FixItHint::CreateRemoval(SourceRange(InitRange.getEnd(), NewEnd));
+    break;
+  }
+  case CXXNewExpr::ListInit: {
+    SourceRange InitRange = New->getInitializer()->getSourceRange();
+    Diag << FixItHint::CreateRemoval(
+        SourceRange(NewStart, InitRange.getBegin().getLocWithOffset(-1)));
+    Diag << FixItHint::CreateRemoval(
+        SourceRange(InitRange.getEnd().getLocWithOffset(1), NewEnd));
+    break;
+  }
+  }
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

clang-tidy/modernize/ModernizeTidyModule.cpp

 //===--- ModernizeTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "LoopConvertCheck.h"
+#include "MakeUniqueCheck.h"
 #include "PassByValueCheck.h"
 #include "ReplaceAutoPtrCheck.h"
 #include "ShrinkToFitCheck.h"
 #include "UseAutoCheck.h"
 #include "UseNullptrCheck.h"
 #include "UseOverrideCheck.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
 class ModernizeModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<LoopConvertCheck>("modernize-loop-convert");
+    CheckFactories.registerCheck<MakeUniqueCheck>(
+        "modernize-make-unique");
     CheckFactories.registerCheck<PassByValueCheck>("modernize-pass-by-value");
     CheckFactories.registerCheck<ReplaceAutoPtrCheck>(
         "modernize-replace-auto-ptr");
     CheckFactories.registerCheck<ShrinkToFitCheck>("modernize-shrink-to-fit");
     CheckFactories.registerCheck<UseAutoCheck>("modernize-use-auto");
     CheckFactories.registerCheck<UseNullptrCheck>("modernize-use-nullptr");
     CheckFactories.registerCheck<UseOverrideCheck>("modernize-use-override");
   }

test/clang-tidy/modernize-make-unique.cpp

+// RUN: %python %S/check_clang_tidy.py %s modernize-make-unique %t
+
+namespace std {
+
+template <typename type>
+class unique_ptr {
+public:
+  unique_ptr(type *ptr);
+  unique_ptr(const unique_ptr<type> &t) = delete;
+  unique_ptr(unique_ptr<type> &&t);
+  ~unique_ptr();
+  type &operator*() { return *ptr; }
+  type *operator->() { return ptr; }
+  type *release();
+  void reset();
+  void reset(type *pt);
+
+private:
+  type *ptr;
+};
+
+}
+
+struct Base {
+  Base();
+  Base(int, int);
+};
+
+struct Derived : public Base {
+  Derived();
+  Derived(int, int);
+};
+
+struct Pair {
+  int a, b;
+};
+
+int g(std::unique_ptr<int> P);
+
+std::unique_ptr<Base> getPointer() {
+  return std::unique_ptr<Base>(new Base);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: use std::make_unique instead
+  // CHECK-FIXES: return std::make_unique<Base>();
+}
+
+void f() {
+  std::unique_ptr<int> P1 = std::unique_ptr<int>(new int());
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use std::make_unique instead [modernize-make-unique]
+  // CHECK-FIXES: std::unique_ptr<int> P1 = std::make_unique<int>();
+
+  // Without parenthesis
+  std::unique_ptr<int> P2 = std::unique_ptr<int>(new int);
+  // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: use std::make_unique instead [modernize-make-unique]
+  // CHECK-FIXES: std::unique_ptr<int> P2 = std::make_unique<int>();
+
+  // With auto.
+  auto P3 = std::unique_ptr<int>(new int());
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use std::make_unique instead
+  // CHECK-FIXES: auto P3 = std::make_unique<int>();
+
+  {
+    // No std.
+    using namespace std;
+    unique_ptr<int> Q = unique_ptr<int>(new int());
+    // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: use std::make_unique instead
+    // CHECK-FIXES: unique_ptr<int> Q = std::make_unique<int>();
+  }
+
+  std::unique_ptr<int> R(new int());
+
+  // Create the unique_ptr as a parameter to a function.
+  int T = g(std::unique_ptr<int>(new int()));
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: use std::make_unique instead
+  // CHECK-FIXES: int T = g(std::make_unique<int>());
+
+  // Arguments are correctly handled.
+  std::unique_ptr<Base> Pbase = std::unique_ptr<Base>(new Base(5, T));
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: use std::make_unique instead
+  // CHECK-FIXES: std::unique_ptr<Base> Pbase = std::make_unique<Base>(5, T);
+
+  // Works with init lists.
+  std::unique_ptr<Pair> Ppair = std::unique_ptr<Pair>(new Pair{T, 1});
+  // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: use std::make_unique instead
+  // CHECK-FIXES: std::unique_ptr<Pair> Ppair = std::make_unique<Pair>({T, 1});
+
+  // Only replace if the type in the template is the same than the type returned
+  // by the new operator.
+  auto Pderived = std::unique_ptr<Base>(new Derived());
+
+  // The pointer is returned by the function, nothing to do.
+  std::unique_ptr<Base> RetPtr = getPointer();
+}