[clang-tools-extra] 8bfc141 - [clang-tidy] Added option to uniqueptr delete release check
Nathan James via cfe-commits
cfe-commits at lists.llvm.org
Mon Mar 1 13:52:23 PST 2021
Author: Nathan James
Date: 2021-03-01T21:52:13Z
New Revision: 8bfc14193170c95a49fca3e66aa077203783a137
URL: https://github.com/llvm/llvm-project/commit/8bfc14193170c95a49fca3e66aa077203783a137
DIFF: https://github.com/llvm/llvm-project/commit/8bfc14193170c95a49fca3e66aa077203783a137.diff
LOG: [clang-tidy] Added option to uniqueptr delete release check
Adds an option, `PreferResetCall`, currently defaulted to `false`, to the check.
When `true` the check will refactor by calling the `reset` member function.
Reviewed By: aaron.ballman
Differential Revision: https://reviews.llvm.org/D97630
Added:
Modified:
clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp
clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst
clang-tools-extra/test/clang-tidy/checkers/readability-uniqueptr-delete-release.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp b/clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp
index 478a3f2f93ba..45194a8e3d97 100644
--- a/clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp
+++ b/clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp
@@ -9,6 +9,8 @@
#include "UniqueptrDeleteReleaseCheck.h"
#include "clang/AST/ASTContext.h"
#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/SourceLocation.h"
#include "clang/Lex/Lexer.h"
using namespace clang::ast_matchers;
@@ -17,50 +19,69 @@ namespace clang {
namespace tidy {
namespace readability {
+void UniqueptrDeleteReleaseCheck::storeOptions(
+ ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "PreferResetCall", PreferResetCall);
+}
+
+UniqueptrDeleteReleaseCheck::UniqueptrDeleteReleaseCheck(
+ StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ PreferResetCall(Options.get("PreferResetCall", false)) {}
+
void UniqueptrDeleteReleaseCheck::registerMatchers(MatchFinder *Finder) {
- auto IsSusbstituted = qualType(anyOf(
- substTemplateTypeParmType(), hasDescendant(substTemplateTypeParmType())));
auto UniquePtrWithDefaultDelete = classTemplateSpecializationDecl(
- hasName("std::unique_ptr"),
- hasTemplateArgument(1, refersToType(qualType(hasDeclaration(cxxRecordDecl(
- hasName("std::default_delete")))))));
+ hasName("::std::unique_ptr"),
+ hasTemplateArgument(1, refersToType(hasDeclaration(cxxRecordDecl(
+ hasName("::std::default_delete"))))));
Finder->addMatcher(
- cxxDeleteExpr(has(ignoringParenImpCasts(cxxMemberCallExpr(
- on(expr(hasType(UniquePtrWithDefaultDelete),
- unless(hasType(IsSusbstituted)))
- .bind("uptr")),
- callee(cxxMethodDecl(hasName("release")))))))
+ cxxDeleteExpr(
+ unless(isInTemplateInstantiation()),
+ has(expr(ignoringParenImpCasts(
+ cxxMemberCallExpr(
+ callee(
+ memberExpr(hasObjectExpression(allOf(
+ unless(isTypeDependent()),
+ anyOf(hasType(UniquePtrWithDefaultDelete),
+ hasType(pointsTo(
+ UniquePtrWithDefaultDelete))))),
+ member(cxxMethodDecl(hasName("release"))))
+ .bind("release_expr")))
+ .bind("release_call")))))
.bind("delete"),
this);
}
void UniqueptrDeleteReleaseCheck::check(
const MatchFinder::MatchResult &Result) {
- const auto *PtrExpr = Result.Nodes.getNodeAs<Expr>("uptr");
- const auto *DeleteExpr = Result.Nodes.getNodeAs<Expr>("delete");
+ const auto *DeleteExpr = Result.Nodes.getNodeAs<CXXDeleteExpr>("delete");
+ const auto *ReleaseExpr = Result.Nodes.getNodeAs<MemberExpr>("release_expr");
+ const auto *ReleaseCallExpr =
+ Result.Nodes.getNodeAs<CXXMemberCallExpr>("release_call");
- if (PtrExpr->getBeginLoc().isMacroID())
+ if (ReleaseExpr->getBeginLoc().isMacroID())
return;
- // Ignore dependent types.
- // It can give us false positives, so we go with false negatives instead to
- // be safe.
- if (PtrExpr->getType()->isDependentType())
- return;
-
- SourceLocation AfterPtr = Lexer::getLocForEndOfToken(
- PtrExpr->getEndLoc(), 0, *Result.SourceManager, getLangOpts());
-
- diag(DeleteExpr->getBeginLoc(),
- "prefer '= nullptr' to 'delete x.release()' to reset unique_ptr<> "
- "objects")
- << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
- DeleteExpr->getBeginLoc(), PtrExpr->getBeginLoc()))
- << FixItHint::CreateReplacement(
- CharSourceRange::getTokenRange(AfterPtr, DeleteExpr->getEndLoc()),
- " = nullptr");
+ auto D =
+ diag(DeleteExpr->getBeginLoc(), "prefer '%select{= nullptr|reset()}0' "
+ "to reset 'unique_ptr<>' objects");
+ D << PreferResetCall << DeleteExpr->getSourceRange()
+ << FixItHint::CreateRemoval(CharSourceRange::getCharRange(
+ DeleteExpr->getBeginLoc(),
+ DeleteExpr->getArgument()->getBeginLoc()));
+ if (PreferResetCall) {
+ D << FixItHint::CreateReplacement(ReleaseExpr->getMemberLoc(), "reset");
+ } else {
+ if (ReleaseExpr->isArrow())
+ D << FixItHint::CreateInsertion(ReleaseExpr->getBase()->getBeginLoc(),
+ "*");
+ D << FixItHint::CreateReplacement(
+ CharSourceRange::getTokenRange(ReleaseExpr->getOperatorLoc(),
+ ReleaseCallExpr->getEndLoc()),
+ " = nullptr");
+ }
}
} // namespace readability
diff --git a/clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h b/clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h
index 3e6f184b58b8..88bb82539ac5 100644
--- a/clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h
+++ b/clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h
@@ -22,10 +22,13 @@ namespace readability {
/// http://clang.llvm.org/extra/clang-tidy/checks/readability-uniqueptr-delete-release.html
class UniqueptrDeleteReleaseCheck : public ClangTidyCheck {
public:
- UniqueptrDeleteReleaseCheck(StringRef Name, ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context) {}
+ UniqueptrDeleteReleaseCheck(StringRef Name, ClangTidyContext *Context);
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+ const bool PreferResetCall;
};
} // namespace readability
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 38b3b5205df1..0c7c95dbce3e 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -101,6 +101,13 @@ Changes in existing checks
Added an option to choose the set of allowed functions.
+- Improved :doc:`readability-uniqueptr-delete-release
+ <clang-tidy/checks/readability-uniqueptr-delete-release>` check.
+
+ Added an option to choose whether to refactor by calling the ``reset`` member
+ function or assignment to ``nullptr``.
+ Added support for pointers to ``std::unique_ptr``.
+
Improvements to include-fixer
-----------------------------
diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst b/clang-tools-extra/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst
index beec1d5fb6cb..b183af717044 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst
@@ -15,3 +15,21 @@ The latter is shorter, simpler and does not require use of raw pointer APIs.
std::unique_ptr<int> P;
P = nullptr;
+
+Options
+-------
+
+.. option:: PreferResetCall
+
+ If `true`, refactor by calling the reset member function instead of
+ assigning to ``nullptr``. Default value is `false`.
+
+ .. code-block:: c++
+
+ std::unique_ptr<int> P;
+ delete P.release();
+
+ // becomes
+
+ std::unique_ptr<int> P;
+ P.reset();
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-uniqueptr-delete-release.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-uniqueptr-delete-release.cpp
index bd51bc62dba8..3ae66239e3fa 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/readability-uniqueptr-delete-release.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability-uniqueptr-delete-release.cpp
@@ -1,5 +1,6 @@
-// RUN: %check_clang_tidy %s readability-uniqueptr-delete-release %t
-
+// RUN: %check_clang_tidy %s readability-uniqueptr-delete-release %t -check-suffix=NULLPTR
+// RUN: %check_clang_tidy %s readability-uniqueptr-delete-release %t -check-suffix=RESET -config='{ \
+// RUN: CheckOptions: [{key: readability-uniqueptr-delete-release.PreferResetCall, value: true}]}'
namespace std {
template <typename T>
struct default_delete {};
@@ -13,6 +14,9 @@ class unique_ptr {
template <typename U, typename E>
unique_ptr(unique_ptr<U, E>&&);
T* release();
+ void reset(T *P = nullptr);
+ T &operator*() const;
+ T *operator->() const;
};
} // namespace std
@@ -21,22 +25,62 @@ std::unique_ptr<int>& ReturnsAUnique();
void Positives() {
std::unique_ptr<int> P;
delete P.release();
- // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer '= nullptr' to 'delete x.release()' to reset unique_ptr<> objects [readability-uniqueptr-delete-release]
- // CHECK-FIXES: {{^}} P = nullptr;
+ // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr' to reset 'unique_ptr<>' objects
+ // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()' to reset 'unique_ptr<>' objects
+ // CHECK-FIXES-NULLPTR: {{^}} P = nullptr;
+ // CHECK-FIXES-RESET: {{^}} P.reset();
auto P2 = P;
delete P2.release();
- // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer '= nullptr' to 'delete x.release()' to reset unique_ptr<> objects [readability-uniqueptr-delete-release]
- // CHECK-FIXES: {{^}} P2 = nullptr;
+ // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr' to reset 'unique_ptr<>' objects
+ // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()' to reset 'unique_ptr<>' objects
+ // CHECK-FIXES-NULLPTR: {{^}} P2 = nullptr;
+ // CHECK-FIXES-RESET: {{^}} P2.reset();
+
+ delete (P2.release());
+ // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
+ // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
+ // CHECK-FIXES-NULLPTR: {{^}} (P2 = nullptr);
+ // CHECK-FIXES-RESET: {{^}} (P2.reset());
std::unique_ptr<int> Array[20];
delete Array[4].release();
- // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer '= nullptr' to 'delete
- // CHECK-FIXES: {{^}} Array[4] = nullptr;
+ // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
+ // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
+ // CHECK-FIXES-NULLPTR: {{^}} Array[4] = nullptr;
+ // CHECK-FIXES-RESET: {{^}} Array[4].reset();
delete ReturnsAUnique().release();
- // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer '= nullptr' to 'delete
- // CHECK-FIXES: {{^}} ReturnsAUnique() = nullptr;
+ // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
+ // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
+ // CHECK-FIXES-NULLPTR: {{^}} ReturnsAUnique() = nullptr;
+ // CHECK-FIXES-RESET: {{^}} ReturnsAUnique().reset();
+
+ std::unique_ptr<int> *P3(&P);
+ delete P3->release();
+ // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
+ // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
+ // CHECK-FIXES-NULLPTR: {{^}} *P3 = nullptr;
+ // CHECK-FIXES-RESET: {{^}} P3->reset();
+
+ std::unique_ptr<std::unique_ptr<int>> P4;
+ delete (*P4).release();
+ // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
+ // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
+ // CHECK-FIXES-NULLPTR: {{^}} (*P4) = nullptr;
+ // CHECK-FIXES-RESET: {{^}} (*P4).reset();
+
+ delete P4->release();
+ // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
+ // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
+ // CHECK-FIXES-NULLPTR: {{^}} *P4 = nullptr;
+ // CHECK-FIXES-RESET: {{^}} P4->reset();
+
+ delete (P4)->release();
+ // CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
+ // CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
+ // CHECK-FIXES-NULLPTR: {{^}} *(P4) = nullptr;
+ // CHECK-FIXES-RESET: {{^}} (P4)->reset();
}
struct NotDefaultDeleter {};
@@ -51,6 +95,9 @@ void Negatives() {
NotUniquePtr P2;
delete P2.release();
+
+ // We don't trigger on bound member function calls.
+ delete (P2.release)();
}
template <typename T, typename D>
More information about the cfe-commits
mailing list