Index: clang-tidy/abseil/AbseilTidyModule.cpp =================================================================== --- clang-tidy/abseil/AbseilTidyModule.cpp +++ clang-tidy/abseil/AbseilTidyModule.cpp @@ -22,6 +22,7 @@ #include "StringFindStartswithCheck.h" #include "StrCatAppendCheck.h" #include "UpgradeDurationConversionsCheck.h" +#include "WrapUniqueCheck.h" namespace clang { namespace tidy { @@ -55,7 +56,9 @@ "abseil-string-find-startswith"); CheckFactories.registerCheck( "abseil-upgrade-duration-conversions"); - } + CheckFactories.registerCheck( + "abseil-wrap-unique"); + } }; // Register the AbseilModule using this statically initialized variable. Index: clang-tidy/abseil/CMakeLists.txt =================================================================== --- clang-tidy/abseil/CMakeLists.txt +++ clang-tidy/abseil/CMakeLists.txt @@ -16,6 +16,7 @@ StrCatAppendCheck.cpp StringFindStartswithCheck.cpp UpgradeDurationConversionsCheck.cpp + WrapUniqueCheck.cpp LINK_LIBS clangAST Index: clang-tidy/abseil/WrapUniqueCheck.h =================================================================== --- /dev/null +++ clang-tidy/abseil/WrapUniqueCheck.h @@ -0,0 +1,40 @@ +//===--- WrapUniqueCheck.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_ABSEIL_WRAPUNIQUECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_WRAPUNIQUECHECK_H + +#include "../ClangTidy.h" + +namespace clang { +namespace tidy { +namespace abseil { + +/// Check for instances of factory functions, which use a non-public constructor, +/// that returns a std::unique_ptr. Then recommends using +/// absl::wrap_unique(new T(...)) +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-wrap-unique.html +class WrapUniqueCheck : public ClangTidyCheck { +private: + std::string getArgs(const SourceManager *SM, const CallExpr *MemExpr); + +public: + WrapUniqueCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; + +} // namespace abseil +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_WRAPUNIQUECHECK_H Index: clang-tidy/abseil/WrapUniqueCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/abseil/WrapUniqueCheck.cpp @@ -0,0 +1,104 @@ +//===--- WrapUniqueCheck.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 "WrapUniqueCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace abseil { + +std::string WrapUniqueCheck::getArgs(const SourceManager *SM, + const CallExpr *MemExpr) { + llvm::StringRef ArgRef = Lexer::getSourceText( + CharSourceRange::getCharRange(MemExpr->getSourceRange()), *SM, + LangOptions()); + + return (ArgRef.str().length() > 0) ? ArgRef.str() + ")" : "()"; +} + +void WrapUniqueCheck::registerMatchers(MatchFinder *Finder) { + Finder->addMatcher( + cxxMemberCallExpr( + callee(cxxMethodDecl( + hasName("reset"), + ofClass(cxxRecordDecl(hasName("std::unique_ptr"), decl())))), + has(memberExpr(has(declRefExpr()))), + has(callExpr(has(implicitCastExpr(has(declRefExpr()))))), + hasArgument(0, callExpr().bind("callExpr"))) + .bind("facCons"), + this); + + Finder->addMatcher( + cxxConstructExpr(anyOf(hasParent(decl().bind("cons_decl")), anything()), + hasType(cxxRecordDecl(hasName("std::unique_ptr"))), + has(callExpr().bind("FC_call"))) + .bind("upfc"), + this); +} + +void WrapUniqueCheck::check(const MatchFinder::MatchResult &Result) { + // gets the instance of factory constructor + const SourceManager *SM = Result.SourceManager; + const auto *facExpr = Result.Nodes.getNodeAs("facCons"); + const auto *callExpr = Result.Nodes.getNodeAs("callExpr"); + + const auto *cons = Result.Nodes.getNodeAs("upfc"); + const auto *consDecl = Result.Nodes.getNodeAs("cons_decl"); + const auto *FC_Call = Result.Nodes.getNodeAs("FC_call"); + + if (facExpr) { + std::string diagText = "Perfer absl::WrapUnique for resetting unique_ptr"; + std::string newText; + + const Expr *ObjectArg = facExpr->getImplicitObjectArgument(); + SourceLocation Target = ObjectArg->getExprLoc(); + llvm::StringRef ObjName = + Lexer::getSourceText(CharSourceRange::getCharRange( + ObjectArg->getBeginLoc(), + facExpr->getExprLoc().getLocWithOffset(-1)), + *SM, LangOptions()); + + newText = + ObjName.str() + " = absl::WrapUnique(" + getArgs(SM, callExpr) + ")"; + + diag(Target, diagText) << FixItHint::CreateReplacement( + CharSourceRange::getTokenRange(Target, facExpr->getEndLoc()), newText); + } + + if (cons) { + if (cons->isListInitialization()) { + return; + } + + std::string diagText = "Perfer absl::WrapUnique to constructing unique_ptr"; + std::string newText; + std::string Left; + + llvm::StringRef NameRef = Lexer::getSourceText( + CharSourceRange::getCharRange(cons->getBeginLoc(), + cons->getParenOrBraceRange().getBegin()), + *SM, LangOptions()); + + Left = (consDecl) ? "auto " + NameRef.str() + " = " : ""; + newText = Left + "absl::WrapUnique(" + getArgs(SM, FC_Call) + ")"; + SourceLocation Target = + (consDecl) ? consDecl->getBeginLoc() : cons->getExprLoc(); + + diag(Target, diagText) << FixItHint::CreateReplacement( + CharSourceRange::getTokenRange(Target, cons->getEndLoc()), newText); + } +} +} // namespace abseil +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -73,6 +73,14 @@ Checks for casts of ``absl::Duration`` conversion functions, and recommends the right conversion function instead. +- New :doc:`abseil-wrap-unique + ` check. + + Looks for instances of factory functions which uses a non-public constructor + that returns a ``std::unqiue_ptr`` then recommends using + ``absl::wrap_unique(new T(...))``. + + Improvements to include-fixer ----------------------------- Index: docs/clang-tidy/checks/abseil-wrap-unique.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/abseil-wrap-unique.rst @@ -0,0 +1,31 @@ +.. title:: clang-tidy - abseil-wrap-unique + +abseil-wrap-unique +================== +Looks for instances of factory functions which uses a non-public constructor +that returns a ``std::unqiue_ptr`` then recommends using +``absl::wrap_unique(new T(...))``. + +.. code-block:: c++ + class A { + public: + static A* NewA() { return new A(); } + + private: + A() {} + }; + + std::unique_ptr a; + + // Original - reset called with a static function returning a std::unqiue_ptr + a.reset(A::NewA()); + + // Suggested - reset ptr with absl::WrapUnique + a = absl::WrapUnique(A::NewA()); + + // Original - std::unique_ptr initialized with static function + std::unique_ptr b(A::NewA()); + + // Suggested - initialize with absl::WrapUnique instead + auto b = absl::WrapUnique(A::NewA()) + Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -17,6 +17,7 @@ abseil-str-cat-append abseil-string-find-startswith abseil-upgrade-duration-conversions + abseil-wrap-unique android-cloexec-accept android-cloexec-accept4 android-cloexec-creat Index: test/clang-tidy/abseil-wrap-unique.cpp =================================================================== --- /dev/null +++ test/clang-tidy/abseil-wrap-unique.cpp @@ -0,0 +1,87 @@ +// RUN: %check_clang_tidy %s abseil-wrap-unique %t + +namespace std { + +template +class default_delete {}; + +template > +class unique_ptr { +public: + unique_ptr() {} + unique_ptr(type *ptr) {} + unique_ptr(const unique_ptr &t) = delete; + unique_ptr(unique_ptr &&t) {} + ~unique_ptr() {} + type &operator*() { return *ptr; } + type *operator->() { return ptr; } + type *release() { return ptr; } + void reset() {} + void reset(type *pt) {} + void reset(type pt) {} + unique_ptr &operator=(unique_ptr &&) { return *this; } + template + unique_ptr &operator=(unique_ptr &&) { return *this; } + +private: + type *ptr; +}; +} // namespace std + +class A { + public: + static A* NewA() { + return new A(); + } + + private: + A() {} +}; + +class B { + public: + static B* NewB(int bIn) { + return new B(); + } + + private: + B() {} +}; + +struct C { + int x; + int y; +}; +/* +std::unique_ptr returnPointer() { + return std::unique_ptr(A::NewA()); +} +*/ +void positives() { + std::unique_ptr a; + a.reset(A::NewA()); + //CHECK-MESSAGE: :[[@LINE-1]]:3: warning: prefer absl::WrapUnique for resetting unique_ptr [abseil-wrap-unique] + //CHECK-FIXES: a = absl::WrapUnique(A::NewA()) + + std::unique_ptr b(A::NewA()); + //CHECK-MESSAGE: :[[@LINE-1]]:3: warning: Perfer absl::WrapUnique to constructing unique_ptr [abseil-wrap-unique] + //CHECK-FIXES: auto b = absl::WrapUnique(A::NewA()) + + int cIn; + std::unique_ptr c(B::NewB(cIn)); + //CHECK-MESSAGE: :[[@LINE-1]]:3: warning: Perfer absl::WrapUnique to constructing unique_ptr [abseil-wrap-unique] + //CHECK-FIXES: auto c = absl::WrapUnique(B::NewB(cIn)) + + int dIn; + std::unique_ptr d; + d.reset(B::NewB(dIn)); + //CHECK-MESSAGE: :[[@LINE-1]]:3: warning: prefer absl::WrapUnique for resetting unique_ptr [abseil-wrap-unique] + //CHECK-FIXES: d = absl::WrapUnique(B::NewB(dIn)) + + auto e = std::unique_ptr(A::NewA()); + //CHECK-MESSAGE: :[[@LINE-1]]:3: warning: prefer absl::WrapUnique for resetting unique_ptr [abseil-wrap-unique] + //CHECK-FIXES: e = absl::WrapUnique(A::NewA()) + + //std::unique_ptr e(new int[2] {1,2}); +} +