Dosi-Dough updated this revision to Diff 190552.
Dosi-Dough marked 11 inline comments as done.
Dosi-Dough added a comment.
Herald added a subscriber: jdoerfert.
Herald added a project: clang.
added style and performance refactoring based on code reviews
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57435/new/
https://reviews.llvm.org/D57435
Files:
clang-tidy/abseil/AbseilTidyModule.cpp
clang-tidy/abseil/CMakeLists.txt
clang-tidy/abseil/WrapUniqueCheck.cpp
clang-tidy/abseil/WrapUniqueCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/abseil-wrap-unique.rst
docs/clang-tidy/checks/list.rst
test/clang-tidy/abseil-wrap-unique.cpp
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 <typename T>
+class default_delete {};
+
+template <typename type, typename Deleter = std::default_delete<type>>
+class unique_ptr {
+public:
+ unique_ptr() {}
+ 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() { return ptr; }
+ void reset() {}
+ void reset(type *pt) {}
+ void reset(type pt) {}
+ unique_ptr &operator=(unique_ptr &&) { return *this; }
+ template <typename T>
+ unique_ptr &operator=(unique_ptr<T> &&) { 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<A> returnPointer() {
+ return std::unique_ptr<A>(A::NewA());
+}
+*/
+void positives() {
+ std::unique_ptr<A> 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<A> 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<B> 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<B> 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>(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<int> e(new int[2] {1,2});
+}
+
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: 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<T>`` 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> 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<A> b(A::NewA());
+
+ // Suggested - initialize with absl::WrapUnique instead
+ auto b = absl::WrapUnique(A::NewA())
+
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
+ <clang-tidy/checks/abseil-wrap-unique>` check.
+
+ Looks for instances of factory functions which uses a non-public constructor
+ that returns a ``std::unqiue_ptr<T>`` then recommends using
+ ``absl::wrap_unique(new T(...))``.
+
- New :doc:`google-readability-avoid-underscore-in-googletest-name
<clang-tidy/checks/google-readability-avoid-underscore-in-googletest-name>`
check.
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<T>. 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,100 @@
+//===--- 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 <string>
+
+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<CXXMemberCallExpr>("facCons");
+ const auto *Cons = Result.Nodes.getNodeAs<CXXConstructExpr>("upfc");
+ if (FacExpr) {
+ const auto *CExpr = Result.Nodes.getNodeAs<CallExpr>("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<CallExpr>("FC_call");
+ const auto *ConsDecl = Result.Nodes.getNodeAs<Decl>("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: 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/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<UpgradeDurationConversionsCheck>(
"abseil-upgrade-duration-conversions");
- }
+ CheckFactories.registerCheck<WrapUniqueCheck>(
+ "abseil-wrap-unique");
+ }
};
// Register the AbseilModule using this statically initialized variable.
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
