Index: clang-tidy/abseil/AbseilTidyModule.cpp =================================================================== --- clang-tidy/abseil/AbseilTidyModule.cpp +++ clang-tidy/abseil/AbseilTidyModule.cpp @@ -25,6 +25,7 @@ #include "StrCatAppendCheck.h" #include "TimeSubtractionCheck.h" #include "UpgradeDurationConversionsCheck.h" +#include "WrapUniqueCheck.h" namespace clang { namespace tidy { @@ -64,7 +65,9 @@ "abseil-time-subtraction"); 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 @@ -19,6 +19,7 @@ StringFindStartswithCheck.cpp TimeSubtractionCheck.cpp UpgradeDurationConversionsCheck.cpp + WrapUniqueCheck.cpp LINK_LIBS clangAST Index: clang-tidy/abseil/WrapUniqueCheck.h =================================================================== --- /dev/null +++ clang-tidy/abseil/WrapUniqueCheck.h @@ -0,0 +1,37 @@ +//===--- UseAutoForRangeCheck.h - clang-tidy --------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#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 { + +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,97 @@ +//===--- WrapUniqueCheck.cpp - clang-tidy ---------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#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 { + +static std::string getArgs(const SourceManager *SM, const CallExpr *MemExpr) { + llvm::StringRef ArgRef = Lexer::getSourceText( + CharSourceRange::getCharRange(MemExpr->getSourceRange()), *SM, + LangOptions()); + + return (!ArgRef.empty()) ? 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 *Cons = Result.Nodes.getNodeAs("upfc"); + if (FacExpr) { + const auto *CExpr = Result.Nodes.getNodeAs("callExpr"); + std::string DiagText = "Perfer absl::WrapUnique for resetting unique_ptr"; + + const Expr *ObjectArg = FacExpr->getImplicitObjectArgument(); + SourceLocation Target = ObjectArg->getExprLoc(); + llvm::StringRef ObjName = + Lexer::getSourceText(CharSourceRange::getCharRange( + ObjectArg->getBeginLoc(), + FacExpr->getExprLoc().getLocWithOffset(-1)), + *SM, LangOptions()); + + std::string NewText = + ObjName.str() + " = absl::WrapUnique(" + getArgs(SM, CExpr) + ")"; + + diag(Target, DiagText) << FixItHint::CreateReplacement( + CharSourceRange::getTokenRange(Target, FacExpr->getEndLoc()), NewText); + } + + if (Cons) { + if (Cons->isListInitialization()) + return; + + const auto *FcCall = Result.Nodes.getNodeAs("FC_call"); + const auto *ConsDecl = Result.Nodes.getNodeAs("cons_decl"); + std::string DiagText = "Perfer absl::WrapUnique to constructing unique_ptr"; + + llvm::StringRef NameRef = Lexer::getSourceText( + CharSourceRange::getCharRange(Cons->getBeginLoc(), + Cons->getParenOrBraceRange().getBegin()), + *SM, LangOptions()); + + std::string Left = ConsDecl ? "auto " + NameRef.str() + " = " : ""; + std::string NewText = + Left + "absl::WrapUnique(" + getArgs(SM, FcCall) + ")"; + 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 @@ -91,6 +91,13 @@ Finds and fixes ``absl::Time`` subtraction expressions to do subtraction in the Time domain instead of the numeric domain. +- 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(...))``. + - New :doc:`google-readability-avoid-underscore-in-googletest-name ` check. Index: docs/clang-tidy/checks/abseil-wrap-unique.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/abseil-wrap-unique.rst @@ -0,0 +1,34 @@ +.. 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(...))``. + +Examples: + +.. code-block:: c++ + + class A { + public: + static A* NewA() { return new A(); } + + private: + A() = default; + }; + + 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 @@ -20,6 +20,7 @@ abseil-string-find-startswith abseil-time-subtraction 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}); +} +