Skip to content

Commit 8bfc141

Browse files
njames93njames93
committed
[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
1 parent 801067f commit 8bfc141

File tree

5 files changed

+138
-42
lines changed

5 files changed

+138
-42
lines changed

‎clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp

Lines changed: 51 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
#include "UniqueptrDeleteReleaseCheck.h"
1010
#include "clang/AST/ASTContext.h"
1111
#include "clang/ASTMatchers/ASTMatchFinder.h"
12+
#include "clang/Basic/Diagnostic.h"
13+
#include "clang/Basic/SourceLocation.h"
1214
#include "clang/Lex/Lexer.h"
1315

1416
using namespace clang::ast_matchers;
@@ -17,50 +19,69 @@ namespace clang {
1719
namespace tidy {
1820
namespace readability {
1921

22+
void UniqueptrDeleteReleaseCheck::storeOptions(
23+
ClangTidyOptions::OptionMap &Opts) {
24+
Options.store(Opts, "PreferResetCall", PreferResetCall);
25+
}
26+
27+
UniqueptrDeleteReleaseCheck::UniqueptrDeleteReleaseCheck(
28+
StringRef Name, ClangTidyContext *Context)
29+
: ClangTidyCheck(Name, Context),
30+
PreferResetCall(Options.get("PreferResetCall", false)) {}
31+
2032
void UniqueptrDeleteReleaseCheck::registerMatchers(MatchFinder *Finder) {
21-
auto IsSusbstituted = qualType(anyOf(
22-
substTemplateTypeParmType(), hasDescendant(substTemplateTypeParmType())));
2333

2434
auto UniquePtrWithDefaultDelete = classTemplateSpecializationDecl(
25-
hasName("std::unique_ptr"),
26-
hasTemplateArgument(1, refersToType(qualType(hasDeclaration(cxxRecordDecl(
27-
hasName("std::default_delete")))))));
35+
hasName("::std::unique_ptr"),
36+
hasTemplateArgument(1, refersToType(hasDeclaration(cxxRecordDecl(
37+
hasName("::std::default_delete"))))));
2838

2939
Finder->addMatcher(
30-
cxxDeleteExpr(has(ignoringParenImpCasts(cxxMemberCallExpr(
31-
on(expr(hasType(UniquePtrWithDefaultDelete),
32-
unless(hasType(IsSusbstituted)))
33-
.bind("uptr")),
34-
callee(cxxMethodDecl(hasName("release")))))))
40+
cxxDeleteExpr(
41+
unless(isInTemplateInstantiation()),
42+
has(expr(ignoringParenImpCasts(
43+
cxxMemberCallExpr(
44+
callee(
45+
memberExpr(hasObjectExpression(allOf(
46+
unless(isTypeDependent()),
47+
anyOf(hasType(UniquePtrWithDefaultDelete),
48+
hasType(pointsTo(
49+
UniquePtrWithDefaultDelete))))),
50+
member(cxxMethodDecl(hasName("release"))))
51+
.bind("release_expr")))
52+
.bind("release_call")))))
3553
.bind("delete"),
3654
this);
3755
}
3856

3957
void UniqueptrDeleteReleaseCheck::check(
4058
const MatchFinder::MatchResult &Result) {
41-
const auto *PtrExpr = Result.Nodes.getNodeAs<Expr>("uptr");
42-
const auto *DeleteExpr = Result.Nodes.getNodeAs<Expr>("delete");
59+
const auto *DeleteExpr = Result.Nodes.getNodeAs<CXXDeleteExpr>("delete");
60+
const auto *ReleaseExpr = Result.Nodes.getNodeAs<MemberExpr>("release_expr");
61+
const auto *ReleaseCallExpr =
62+
Result.Nodes.getNodeAs<CXXMemberCallExpr>("release_call");
4363

44-
if (PtrExpr->getBeginLoc().isMacroID())
64+
if (ReleaseExpr->getBeginLoc().isMacroID())
4565
return;
4666

47-
// Ignore dependent types.
48-
// It can give us false positives, so we go with false negatives instead to
49-
// be safe.
50-
if (PtrExpr->getType()->isDependentType())
51-
return;
52-
53-
SourceLocation AfterPtr = Lexer::getLocForEndOfToken(
54-
PtrExpr->getEndLoc(), 0, *Result.SourceManager, getLangOpts());
55-
56-
diag(DeleteExpr->getBeginLoc(),
57-
"prefer '= nullptr' to 'delete x.release()' to reset unique_ptr<> "
58-
"objects")
59-
<< FixItHint::CreateRemoval(CharSourceRange::getCharRange(
60-
DeleteExpr->getBeginLoc(), PtrExpr->getBeginLoc()))
61-
<< FixItHint::CreateReplacement(
62-
CharSourceRange::getTokenRange(AfterPtr, DeleteExpr->getEndLoc()),
63-
" = nullptr");
67+
auto D =
68+
diag(DeleteExpr->getBeginLoc(), "prefer '%select{= nullptr|reset()}0' "
69+
"to reset 'unique_ptr<>' objects");
70+
D << PreferResetCall << DeleteExpr->getSourceRange()
71+
<< FixItHint::CreateRemoval(CharSourceRange::getCharRange(
72+
DeleteExpr->getBeginLoc(),
73+
DeleteExpr->getArgument()->getBeginLoc()));
74+
if (PreferResetCall) {
75+
D << FixItHint::CreateReplacement(ReleaseExpr->getMemberLoc(), "reset");
76+
} else {
77+
if (ReleaseExpr->isArrow())
78+
D << FixItHint::CreateInsertion(ReleaseExpr->getBase()->getBeginLoc(),
79+
"*");
80+
D << FixItHint::CreateReplacement(
81+
CharSourceRange::getTokenRange(ReleaseExpr->getOperatorLoc(),
82+
ReleaseCallExpr->getEndLoc()),
83+
" = nullptr");
84+
}
6485
}
6586

6687
} // namespace readability

‎clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.h

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,13 @@ namespace readability {
2222
/// http://clang.llvm.org/extra/clang-tidy/checks/readability-uniqueptr-delete-release.html
2323
class UniqueptrDeleteReleaseCheck : public ClangTidyCheck {
2424
public:
25-
UniqueptrDeleteReleaseCheck(StringRef Name, ClangTidyContext *Context)
26-
: ClangTidyCheck(Name, Context) {}
25+
UniqueptrDeleteReleaseCheck(StringRef Name, ClangTidyContext *Context);
2726
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
2827
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
28+
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
29+
30+
private:
31+
const bool PreferResetCall;
2932
};
3033

3134
} // namespace readability

‎clang-tools-extra/docs/ReleaseNotes.rst

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,13 @@ Changes in existing checks
101101

102102
Added an option to choose the set of allowed functions.
103103

104+
- Improved :doc:`readability-uniqueptr-delete-release
105+
<clang-tidy/checks/readability-uniqueptr-delete-release>` check.
106+
107+
Added an option to choose whether to refactor by calling the ``reset`` member
108+
function or assignment to ``nullptr``.
109+
Added support for pointers to ``std::unique_ptr``.
110+
104111
Improvements to include-fixer
105112
-----------------------------
106113

‎clang-tools-extra/docs/clang-tidy/checks/readability-uniqueptr-delete-release.rst

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,3 +15,21 @@ The latter is shorter, simpler and does not require use of raw pointer APIs.
1515

1616
std::unique_ptr<int> P;
1717
P = nullptr;
18+
19+
Options
20+
-------
21+
22+
.. option:: PreferResetCall
23+
24+
If `true`, refactor by calling the reset member function instead of
25+
assigning to ``nullptr``. Default value is `false`.
26+
27+
.. code-block:: c++
28+
29+
std::unique_ptr<int> P;
30+
delete P.release();
31+
32+
// becomes
33+
34+
std::unique_ptr<int> P;
35+
P.reset();

‎clang-tools-extra/test/clang-tidy/checkers/readability-uniqueptr-delete-release.cpp

Lines changed: 57 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1-
// RUN: %check_clang_tidy %s readability-uniqueptr-delete-release %t
2-
1+
// RUN: %check_clang_tidy %s readability-uniqueptr-delete-release %t -check-suffix=NULLPTR
2+
// RUN: %check_clang_tidy %s readability-uniqueptr-delete-release %t -check-suffix=RESET -config='{ \
3+
// RUN: CheckOptions: [{key: readability-uniqueptr-delete-release.PreferResetCall, value: true}]}'
34
namespace std {
45
template <typename T>
56
struct default_delete {};
@@ -13,6 +14,9 @@ class unique_ptr {
1314
template <typename U, typename E>
1415
unique_ptr(unique_ptr<U, E>&&);
1516
T* release();
17+
void reset(T *P = nullptr);
18+
T &operator*() const;
19+
T *operator->() const;
1620
};
1721
} // namespace std
1822

@@ -21,22 +25,62 @@ std::unique_ptr<int>& ReturnsAUnique();
2125
void Positives() {
2226
std::unique_ptr<int> P;
2327
delete P.release();
24-
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer '= nullptr' to 'delete x.release()' to reset unique_ptr<> objects [readability-uniqueptr-delete-release]
25-
// CHECK-FIXES: {{^}} P = nullptr;
28+
// CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr' to reset 'unique_ptr<>' objects
29+
// CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()' to reset 'unique_ptr<>' objects
30+
// CHECK-FIXES-NULLPTR: {{^}} P = nullptr;
31+
// CHECK-FIXES-RESET: {{^}} P.reset();
2632

2733
auto P2 = P;
2834
delete P2.release();
29-
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer '= nullptr' to 'delete x.release()' to reset unique_ptr<> objects [readability-uniqueptr-delete-release]
30-
// CHECK-FIXES: {{^}} P2 = nullptr;
35+
// CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr' to reset 'unique_ptr<>' objects
36+
// CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()' to reset 'unique_ptr<>' objects
37+
// CHECK-FIXES-NULLPTR: {{^}} P2 = nullptr;
38+
// CHECK-FIXES-RESET: {{^}} P2.reset();
39+
40+
delete (P2.release());
41+
// CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
42+
// CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
43+
// CHECK-FIXES-NULLPTR: {{^}} (P2 = nullptr);
44+
// CHECK-FIXES-RESET: {{^}} (P2.reset());
3145

3246
std::unique_ptr<int> Array[20];
3347
delete Array[4].release();
34-
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer '= nullptr' to 'delete
35-
// CHECK-FIXES: {{^}} Array[4] = nullptr;
48+
// CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
49+
// CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
50+
// CHECK-FIXES-NULLPTR: {{^}} Array[4] = nullptr;
51+
// CHECK-FIXES-RESET: {{^}} Array[4].reset();
3652

3753
delete ReturnsAUnique().release();
38-
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer '= nullptr' to 'delete
39-
// CHECK-FIXES: {{^}} ReturnsAUnique() = nullptr;
54+
// CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
55+
// CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
56+
// CHECK-FIXES-NULLPTR: {{^}} ReturnsAUnique() = nullptr;
57+
// CHECK-FIXES-RESET: {{^}} ReturnsAUnique().reset();
58+
59+
std::unique_ptr<int> *P3(&P);
60+
delete P3->release();
61+
// CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
62+
// CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
63+
// CHECK-FIXES-NULLPTR: {{^}} *P3 = nullptr;
64+
// CHECK-FIXES-RESET: {{^}} P3->reset();
65+
66+
std::unique_ptr<std::unique_ptr<int>> P4;
67+
delete (*P4).release();
68+
// CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
69+
// CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
70+
// CHECK-FIXES-NULLPTR: {{^}} (*P4) = nullptr;
71+
// CHECK-FIXES-RESET: {{^}} (*P4).reset();
72+
73+
delete P4->release();
74+
// CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
75+
// CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
76+
// CHECK-FIXES-NULLPTR: {{^}} *P4 = nullptr;
77+
// CHECK-FIXES-RESET: {{^}} P4->reset();
78+
79+
delete (P4)->release();
80+
// CHECK-MESSAGES-NULLPTR: :[[@LINE-1]]:3: warning: prefer '= nullptr'
81+
// CHECK-MESSAGES-RESET: :[[@LINE-2]]:3: warning: prefer 'reset()'
82+
// CHECK-FIXES-NULLPTR: {{^}} *(P4) = nullptr;
83+
// CHECK-FIXES-RESET: {{^}} (P4)->reset();
4084
}
4185

4286
struct NotDefaultDeleter {};
@@ -51,6 +95,9 @@ void Negatives() {
5195

5296
NotUniquePtr P2;
5397
delete P2.release();
98+
99+
// We don't trigger on bound member function calls.
100+
delete (P2.release)();
54101
}
55102

56103
template <typename T, typename D>

0 commit comments

Comments
 (0)