[clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check #121291
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Baranov Victor (vbvictor) ChangesAdd new clang-tidy check that finds potentially erroneous calls to It's easy to make typo and delete object because the difference between Sometimes IDE's autocomplete will change Small example: struct Resettable {
void reset() { /* Own reset logic */ }
};
auto ptr = std::make_unique<Resettable>();
ptr->reset(); // Calls underlying reset method
ptr.reset(); // Makes the pointer null
// After Fix-its
(*ptr).reset(); // Clearly calls underlying reset method
ptr = nullptr; // Clearly makes the pointer nullFull diff: https://github.com/llvm/llvm-project/pull/121291.diff 8 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
index 33ac65e715ce81..645958e47e22a5 100644
--- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -57,6 +57,7 @@
#include "PosixReturnCheck.h"
#include "RedundantBranchConditionCheck.h"
#include "ReservedIdentifierCheck.h"
+#include "ResetCallCheck.h"
#include "ReturnConstRefFromParameterCheck.h"
#include "SharedPtrArrayMismatchCheck.h"
#include "SignalHandlerCheck.h"
@@ -144,6 +145,7 @@ class BugproneModule : public ClangTidyModule {
"bugprone-inaccurate-erase");
CheckFactories.registerCheck<IncorrectEnableIfCheck>(
"bugprone-incorrect-enable-if");
+ CheckFactories.registerCheck<ResetCallCheck>("bugprone-reset-call");
CheckFactories.registerCheck<ReturnConstRefFromParameterCheck>(
"bugprone-return-const-ref-from-parameter");
CheckFactories.registerCheck<SwitchMissingDefaultCaseCheck>(
diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
index 13adad7c3dadbd..17ab5b27ec5550 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
@@ -52,6 +52,7 @@ add_clang_library(clangTidyBugproneModule STATIC
PosixReturnCheck.cpp
RedundantBranchConditionCheck.cpp
ReservedIdentifierCheck.cpp
+ ResetCallCheck.cpp
ReturnConstRefFromParameterCheck.cpp
SharedPtrArrayMismatchCheck.cpp
SignalHandlerCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.cpp
new file mode 100644
index 00000000000000..305ac8d51adf3e
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.cpp
@@ -0,0 +1,133 @@
+//===--- ResetCallCheck.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 "ResetCallCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::bugprone {
+
+namespace {
+
+AST_MATCHER_P(CallExpr, everyArgumentMatches,
+ ast_matchers::internal::Matcher<Expr>, InnerMatcher) {
+ if (Node.getNumArgs() == 0)
+ return true;
+
+ for (const auto *Arg : Node.arguments()) {
+ if (!InnerMatcher.matches(*Arg, Finder, Builder))
+ return false;
+ }
+ return true;
+}
+
+AST_MATCHER(CXXMethodDecl, hasOnlyDefaultArgs) {
+ if (Node.param_empty())
+ return true;
+
+ for (const auto *Param : Node.parameters()) {
+ if (!Param->hasDefaultArg())
+ return false;
+ }
+ return true;
+}
+
+} // namespace
+
+void ResetCallCheck::registerMatchers(MatchFinder *Finder) {
+ const auto IsSmartptr = hasAnyName("::std::unique_ptr", "::std::shared_ptr");
+
+ const auto ResetMethod =
+ cxxMethodDecl(hasName("reset"), hasOnlyDefaultArgs());
+
+ const auto TypeWithReset =
+ anyOf(cxxRecordDecl(hasMethod(ResetMethod)),
+ classTemplateSpecializationDecl(
+ hasSpecializedTemplate(classTemplateDecl(has(ResetMethod)))));
+
+ const auto SmartptrWithBugproneReset = classTemplateSpecializationDecl(
+ IsSmartptr,
+ hasTemplateArgument(
+ 0, templateArgument(refersToType(hasUnqualifiedDesugaredType(
+ recordType(hasDeclaration(TypeWithReset)))))));
+
+ // Find a.reset() calls
+ Finder->addMatcher(
+ cxxMemberCallExpr(callee(memberExpr(member(hasName("reset")))),
+ everyArgumentMatches(cxxDefaultArgExpr()),
+ on(expr(hasType(SmartptrWithBugproneReset))))
+ .bind("smartptrResetCall"),
+ this);
+
+ // Find a->reset() calls
+ Finder->addMatcher(
+ cxxMemberCallExpr(
+ callee(memberExpr(
+ member(ResetMethod),
+ hasObjectExpression(
+ cxxOperatorCallExpr(
+ hasOverloadedOperatorName("->"),
+ hasArgument(
+ 0, expr(hasType(
+ classTemplateSpecializationDecl(IsSmartptr)))))
+ .bind("OpCall")))),
+ everyArgumentMatches(cxxDefaultArgExpr()))
+ .bind("objectResetCall"),
+ this);
+}
+
+void ResetCallCheck::check(const MatchFinder::MatchResult &Result) {
+ const auto *SmartptrResetCall =
+ Result.Nodes.getNodeAs<CXXMemberCallExpr>("smartptrResetCall");
+ const auto *ObjectResetCall =
+ Result.Nodes.getNodeAs<CXXMemberCallExpr>("objectResetCall");
+
+ if (SmartptrResetCall) {
+ const auto *Member = cast<MemberExpr>(SmartptrResetCall->getCallee());
+
+ auto DiagBuilder = diag(SmartptrResetCall->getBeginLoc(),
+ "bugprone 'reset()' call on smart pointer");
+
+ DiagBuilder << FixItHint::CreateReplacement(
+ SourceRange(Member->getOperatorLoc(),
+ Member->getOperatorLoc().getLocWithOffset(0)),
+ " =");
+
+ DiagBuilder << FixItHint::CreateReplacement(
+ SourceRange(Member->getMemberLoc(), SmartptrResetCall->getEndLoc()),
+ " nullptr");
+ }
+
+ if (ObjectResetCall) {
+ const auto *Arrow = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("OpCall");
+
+ const auto SmartptrSourceRange =
+ Lexer::getAsCharRange(Arrow->getArg(0)->getSourceRange(),
+ *Result.SourceManager, getLangOpts());
+
+ auto DiagBuilder = diag(ObjectResetCall->getBeginLoc(),
+ "bugprone 'reset()' call on smart pointer");
+
+ DiagBuilder << FixItHint::CreateInsertion(SmartptrSourceRange.getBegin(),
+ "(*");
+ DiagBuilder << FixItHint::CreateInsertion(SmartptrSourceRange.getEnd(),
+ ")");
+
+ DiagBuilder << FixItHint::CreateReplacement(
+ CharSourceRange::getCharRange(
+ Arrow->getOperatorLoc(),
+ Arrow->getOperatorLoc().getLocWithOffset(2)),
+ ".");
+ }
+}
+
+} // namespace clang::tidy::bugprone
diff --git a/clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.h b/clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.h
new file mode 100644
index 00000000000000..18f6ee4f6bed22
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.h
@@ -0,0 +1,34 @@
+//===--- ResetCallCheck.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_BUGPRONE_RESETCALLCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_RESETCALLCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::bugprone {
+
+/// Finds potentially erroneous calls to 'reset()' method on smart pointers when
+/// the pointee type also has a 'reset()' method
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/reset-call.html
+class ResetCallCheck : public ClangTidyCheck {
+public:
+ ResetCallCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+ return LangOpts.CPlusPlus;
+ }
+};
+
+} // namespace clang::tidy::bugprone
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_RESETCALLCHECK_H
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index fa3a8e577a33ad..d125afef0bb039 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -148,6 +148,12 @@ New checks
Finds nondeterministic usages of pointers in unordered containers.
+- New :doc:`bugprone-reset-call
+ <clang-tidy/checks/bugprone/reset-call>` check.
+
+ Finds potentially erroneous calls to ``reset()`` method on smart pointers when
+ the pointee type also has a ``reset()`` method.
+
- New :doc:`bugprone-tagged-union-member-count
<clang-tidy/checks/bugprone/tagged-union-member-count>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/reset-call.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/reset-call.rst
new file mode 100644
index 00000000000000..87cf955b26b648
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/reset-call.rst
@@ -0,0 +1,33 @@
+.. title:: clang-tidy - bugprone-reset-call
+
+bugprone-reset-call
+===================
+
+Finds calls to ``reset()`` method on smart pointers where the pointee type
+also has a ``reset()`` method, which makes it easy to accidentally
+make the pointer null when intending to reset the underlying object.
+
+.. code-block:: c++
+
+ struct Resettable {
+ void reset() { /* Own reset logic */ }
+ };
+
+ auto ptr = std::make_unique<Resettable>();
+
+ ptr->reset(); // Calls underlying reset method
+ ptr.reset(); // Makes the pointer null
+
+Both calls are valid C++ code, but the second one might not be
+what the developer intended, as it destroys the pointed-to object
+rather than resetting its state.
+It's easy to make such typo because the difference between ``.`` and ``->`` is really small.
+
+The recommended approach is to make the intent explicit by using either member access or direct assignment:
+
+.. code-block:: c++
+
+ std::unique_ptr<Resettable> ptr = std::make_unique<Resettable>();
+
+ (*ptr).reset(); // Clearly calls underlying reset method
+ ptr = nullptr; // Clearly makes the pointer null
\ No newline at end of file
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index 4d8853a0f6d86c..9ca866da74666d 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -124,6 +124,7 @@ Clang-Tidy Checks
:doc:`bugprone-posix-return <bugprone/posix-return>`, "Yes"
:doc:`bugprone-redundant-branch-condition <bugprone/redundant-branch-condition>`, "Yes"
:doc:`bugprone-reserved-identifier <bugprone/reserved-identifier>`, "Yes"
+ :doc:`bugprone-reset-call <bugprone/reset-call>`, "Yes"
:doc:`bugprone-return-const-ref-from-parameter <bugprone/return-const-ref-from-parameter>`,
:doc:`bugprone-shared-ptr-array-mismatch <bugprone/shared-ptr-array-mismatch>`, "Yes"
:doc:`bugprone-signal-handler <bugprone/signal-handler>`,
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/reset-call.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/reset-call.cpp
new file mode 100644
index 00000000000000..d98f4b463840c7
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/reset-call.cpp
@@ -0,0 +1,215 @@
+// RUN: %check_clang_tidy %s bugprone-reset-call %t
+
+namespace std {
+
+template <typename T>
+struct unique_ptr {
+ T& operator*() const;
+ T* operator->() const;
+ void reset(T* p = nullptr);
+};
+
+template <typename T>
+struct shared_ptr {
+ T& operator*() const;
+ T* operator->() const;
+ void reset();
+ void reset(T*);
+};
+
+} // namespace std
+
+struct Resettable {
+ void reset();
+ void doSomething();
+};
+
+struct ResettableWithParam {
+ void reset(int a);
+ void doSomething();
+};
+
+struct ResettableWithDefaultParams {
+ void reset(int a = 0, double b = 0.0);
+ void doSomething();
+};
+
+struct NonResettable {
+ void doSomething();
+};
+
+void Positive() {
+ std::unique_ptr<Resettable> u;
+ u.reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: u = nullptr;
+ u->reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: (*u).reset();
+
+ std::shared_ptr<Resettable> s;
+ s.reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: s = nullptr;
+ s->reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: (*s).reset();
+
+ std::unique_ptr<std::unique_ptr<int>> uu_ptr;
+ uu_ptr.reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: uu_ptr = nullptr;
+ uu_ptr->reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: (*uu_ptr).reset();
+
+ std::unique_ptr<std::shared_ptr<int>> su_ptr;
+ su_ptr.reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: su_ptr = nullptr;
+ su_ptr->reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: (*su_ptr).reset();
+
+ std::unique_ptr<ResettableWithDefaultParams> rd;
+ rd.reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: rd = nullptr;
+ rd->reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: (*rd).reset();
+
+ std::unique_ptr<std::shared_ptr<std::unique_ptr<Resettable>>> nested_ptr;
+ nested_ptr.reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: nested_ptr = nullptr;
+ nested_ptr->reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: (*nested_ptr).reset();
+ (*nested_ptr).reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: (*nested_ptr) = nullptr;
+ (*nested_ptr)->reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: (*(*nested_ptr)).reset();
+ (**nested_ptr).reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: (**nested_ptr) = nullptr;
+ (**nested_ptr)->reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: (*(**nested_ptr)).reset();
+}
+
+void Negative() {
+ std::unique_ptr<Resettable> u_ptr;
+ u_ptr.reset(nullptr);
+ u_ptr->doSomething();
+
+ std::shared_ptr<Resettable> s_ptr;
+ s_ptr.reset(nullptr);
+ s_ptr->doSomething();
+
+ Resettable* raw_ptr;
+ raw_ptr->reset();
+ raw_ptr->doSomething();
+
+ Resettable resettable;
+ resettable.reset();
+ resettable.doSomething();
+
+ std::unique_ptr<ResettableWithParam> u_ptr_param;
+ u_ptr_param.reset();
+ u_ptr_param.reset(nullptr);
+ u_ptr_param->reset(0);
+
+ std::unique_ptr<NonResettable> u_ptr_no_reset;
+ u_ptr_no_reset.reset();
+
+ std::shared_ptr<ResettableWithParam> s_ptr_param;
+ s_ptr_param.reset();
+ s_ptr_param->reset(0);
+ s_ptr_param->doSomething();
+
+ std::shared_ptr<NonResettable> s_ptr_no_reset;
+ s_ptr_no_reset.reset();
+
+ std::unique_ptr<ResettableWithDefaultParams> u_ptr_default_params;
+ u_ptr_default_params.reset(nullptr);
+ u_ptr_default_params->reset(1);
+ u_ptr_default_params->reset(1, 2.0);
+}
+
+template <typename T>
+void TemplatePositiveTest() {
+ std::unique_ptr<T> u_ptr;
+
+ u_ptr.reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: u_ptr = nullptr;
+ u_ptr->reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: (*u_ptr).reset();
+
+ std::shared_ptr<T> s_ptr;
+ s_ptr.reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: s_ptr = nullptr;
+ s_ptr->reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: (*s_ptr).reset();
+}
+
+template <typename T>
+void TemplatNegativeTestTypeWithReset() {
+ std::unique_ptr<T> u_ptr;
+ u_ptr.reset();
+ u_ptr->reset(0);
+
+ std::shared_ptr<T> s_ptr;
+ s_ptr.reset();
+ s_ptr->reset(0);
+}
+
+template <typename T>
+void TemplatNegativeTestTypeWithoutReset() {
+ std::unique_ptr<T> u_ptr;
+ u_ptr.reset();
+
+ std::unique_ptr<T> s_ptr;
+ s_ptr.reset();
+}
+
+void instantiate() {
+ TemplatePositiveTest<Resettable>();
+ TemplatePositiveTest<std::unique_ptr<int>>();
+ TemplatePositiveTest<std::shared_ptr<int>>();
+ TemplatNegativeTestTypeWithReset<ResettableWithParam>();
+ TemplatNegativeTestTypeWithoutReset<NonResettable>();
+}
+
+struct S {
+ void foo() {
+ u_ptr.reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: u_ptr = nullptr;
+ u_ptr->reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: (*u_ptr).reset();
+ u_ptr.reset(nullptr);
+ u_ptr->doSomething();
+
+ s_ptr.reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: s_ptr = nullptr;
+ s_ptr->reset();
+ // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: bugprone 'reset()' call on smart pointer
+ // CHECK-FIXES: (*s_ptr).reset();
+ s_ptr.reset(nullptr);
+
+ ptr.reset();
+ }
+
+ std::unique_ptr<Resettable> u_ptr;
+ std::unique_ptr<std::shared_ptr<int>> s_ptr;
+ std::unique_ptr<NonResettable> ptr;
+};
|
clang-tools-extra/docs/clang-tidy/checks/bugprone/reset-call.rst
Outdated
Show resolved
Hide resolved
EugeneZelenko
requested review from
PiotrZSL,
carlosgalvezp,
5chmidti and
HerrCai0907
There was a problem hiding this comment.
- Consider TK_IgnoreUnlessSpelledInSource, may simplify matchers
- Name is too generic, consider:
- bugprone-smartptr-reset-pointee-reset
- bugprone-smartptr-reset-call
- bugprone-smartptr-pointee-reset
- bugprone-smartptr-reset-ambiguous-call (my prefered)
Or any simillar.
vbvictor
changed the title
|
There was a problem hiding this comment.
I think that this check is one that we don't want to emit fixes for, at least not in the main diagnostic, but maybe as a fixit attached to a note. Sort of like:
warning: ...
note: (be more explicit and) use '= nullptr'
The diagnostic message is also too generic and does not say what the problem is. One potential wording could be: be explicit when calling 'reset()' on a smart-pointer with a pointee that has a 'reset()' method
What do others think?
clang-tools-extra/docs/clang-tidy/checks/bugprone/smartptr-reset-ambiguous-call.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/smartptr-reset-ambiguous-call.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/smartptr-reset-ambiguous-call.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.h
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp
Outdated
Show resolved
Hide resolved
I think the case when ptr.reset();
// Converts to
ptr = nullptr;A
I agree that my wording is a bit confusing, so I will change it to a better one when fixit format is settled. |
clang-tools-extra/docs/clang-tidy/checks/bugprone/smartptr-reset-ambiguous-call.rst
Outdated
Show resolved
Hide resolved
|
Thank you everyone for the feedback! @PiotrZSL I tried using TK_IgnoreUnlessSpelledInSource with template <typename T>
void TemplatePositiveTest() {
std::unique_ptr<T> u_ptr;
u_ptr.reset();
u_ptr->reset();
}
void instantiate() {
TemplatePositiveTest<std::unique_ptr<int>>();
}As for now, I'm thinking about leaving it as is and make an NFC change in the future when I get more familiar with matchers. @5chmidti I fixed all your comments and added better warning and note messages: s.reset(); // warning: be explicit when calling 'reset()' on a smart pointer with a pointee that has a 'reset()' method
// note: assign the pointer to 'nullptr'
s->reset(); // warning: be explicit when calling 'reset()' on a pointee of a smart pointer
// note: use dereference to call 'reset' method of the pointee@EugeneZelenko Fixed all pr comments. |
clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/smartptr-reset-ambiguous-call.rst
Outdated
Show resolved
Hide resolved
|
Ping |
|
Please rebase from |
vbvictor
force-pushed
the
bugprone-reset-call
branch
from
804deb7 to
42e03bb
Compare
|
Ping |
There was a problem hiding this comment.
- Overall ok, please work a little bit on an warning message.
- Probably better name would be: readability-ambiguous-smartptr-reset-call
- Same issue exist on other objects like std::optional, and other methods like std::optionalstd::set and call to
emplace. Maybe more checks or more generic check could be constructed where configuration is an pair of "object" and "method".
Consider points 1 & 2. And after that it could be delivered.
| Finder->addMatcher( | ||
| cxxMemberCallExpr(callee(memberExpr(member(hasName("reset")))), | ||
| everyArgumentMatches(cxxDefaultArgExpr()), | ||
| on(expr(hasType(SmartptrWithBugproneReset)))) | ||
| .bind("smartptrResetCall"), | ||
| this); | ||
|
|
||
| // Find a->reset() calls | ||
| Finder->addMatcher( |
There was a problem hiding this comment.
you could merge both addMatcher by using "anyOf".
There was a problem hiding this comment.
I Could not find a good way to use "anyOf" in this context.
If I write like this, it gives CE.
Finder->addMatcher(anyOf(...));If I write:
Finder->addMatcher(
cxxMemberCallExpr(
anyOf(...))
.bind("MemberCall"));I get only one bind-name for cxxMemberCallExpr which complicates futher processing.
If there aren't any performance issues, I'd like to leave as is.
There was a problem hiding this comment.
maybe something like this:
Finder->addMatcher(
cxxMemberCallExpr(
callee(ResetMethod),
unless(hasAnyArgument(expr(unless(cxxDefaultArgExpr())))),
anyOf(
thisPointerType(SmartptrWithBugproneReset),
on(cxxOperatorCallExpr(
hasOverloadedOperatorName("->"),
hasArgument(
0, expr(hasType(classTemplateSpecializationDecl(IsSmartptr)))))
)
)
), this);
There was a problem hiding this comment.
With some modifications made this work,
changed thisPointerType() to on(expr()) because it failed to match some cases with nested ptr's like
std::unique_ptr<std::shared_ptr<std::unique_ptr<Resettable>>> nested_ptr;
clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.h
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/bugprone/smartptr-reset-ambiguous-call.rst
Outdated
Show resolved
Hide resolved
|
@PiotrZSL, thank you for pointing out better naming pattern for diag-messages, would use it in the future.
|
clang-tools-extra/docs/clang-tidy/checks/readability/ambiguous-smartptr-reset-call.rst
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/readability/ambiguous-smartptr-reset-call.rst
Outdated
Show resolved
Hide resolved
| Finder->addMatcher( | ||
| cxxMemberCallExpr(callee(memberExpr(member(hasName("reset")))), | ||
| everyArgumentMatches(cxxDefaultArgExpr()), | ||
| on(expr(hasType(SmartptrWithBugproneReset)))) | ||
| .bind("smartptrResetCall"), | ||
| this); | ||
|
|
||
| // Find a->reset() calls | ||
| Finder->addMatcher( |
There was a problem hiding this comment.
maybe something like this:
Finder->addMatcher(
cxxMemberCallExpr(
callee(ResetMethod),
unless(hasAnyArgument(expr(unless(cxxDefaultArgExpr())))),
anyOf(
thisPointerType(SmartptrWithBugproneReset),
on(cxxOperatorCallExpr(
hasOverloadedOperatorName("->"),
hasArgument(
0, expr(hasType(classTemplateSpecializationDecl(IsSmartptr)))))
)
)
), this);
clang-tools-extra/test/clang-tidy/checkers/readability/ambiguous-smartptr-reset-call.cpp
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/readability/AmbiguousSmartptrResetCallCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/readability/ambiguous-smartptr-reset-call.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/readability/AmbiguousSmartptrResetCallCheck.cpp
Outdated
Show resolved
Hide resolved
|
@PiotrZSL, @denzor200 |
clang-tools-extra/clang-tidy/readability/AmbiguousSmartptrResetCallCheck.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/readability/AmbiguousSmartptrResetCallCheck.cpp
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/readability/ambiguous-smartptr-reset-call.rst
Show resolved
Hide resolved
...extra/test/clang-tidy/checkers/readability/ambiguous-smartptr-reset-call-custom-pointers.cpp
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/readability/ambiguous-smartptr-reset-call.rst
Show resolved
Hide resolved
| @@ -0,0 +1,68 @@ | |||
| // RUN: %check_clang_tidy %s readability-ambiguous-smartptr-reset-call %t \ | |||
There was a problem hiding this comment.
you could merge this file into main test, just have 2 RUN commands there and specify custom suffix for this one (you may need to include current default values also)
There was a problem hiding this comment.
I'd prefer to leave two separate files, they are easier to read and maintain IMHO.
vbvictor
force-pushed
the
bugprone-reset-call
branch
from
81dd8b0 to
099aacc
Compare
|
Since all approves are made, can we land this PR in main? |
|
@HerrCai0907, could you please address #121291 (comment). Thank you! |
If you have at least three merged pull requests, you could request commit access using other ones as example. |
|
@vbvictor If you don't want this to be merged as '70346889+vbvictor@users.noreply.github.com', please change your email privacy settings. |
Thank you for noticing! |
|
@vbvictor Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Add new clang-tidy check that finds potentially erroneous calls to
reset()method on smart pointers whenthe pointee type also has a
reset()method.It's easy to make typo and delete object because the difference between
.and->is really small.Sometimes IDE's autocomplete will change
->to.automatically. For example, developer wroteptr->resbut after pressing Tab it becameptr.reset().Small example:
Fixes #120908