[clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check #121291
+428
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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;
+};
|
| bugprone-reset-call | ||
| =================== | ||
|
|
||
| Finds calls to ``reset()`` method on smart pointers where the pointee type |
There was a problem hiding this comment.
Please make this statement same as statement in Release Notes.
There was a problem hiding this comment.
Done, also removed parentheses in reset to follow general style
EugeneZelenko
requested review from
PiotrZSL,
carlosgalvezp,
5chmidti and
HerrCai0907
PiotrZSL
requested changes
Dec 29, 2024
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.
… to bugprone-smartptr-reset-ambiguous-call, make first statement in docs same as in Release Notes
… parentheses in method name from docs
vbvictor
changed the title
|
5chmidti
requested changes
Dec 30, 2024
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?
Comment on lines
+25
to
+27
| 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: |
There was a problem hiding this comment.
Please wrap lines to 80 chars
|
|
||
| Finds potentially erroneous calls to ``reset`` method on | ||
| smart pointers when the pointee type also has a ``reset`` method. | ||
| Having ``reset`` method in both classes makes it easy to accidentally |
| 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. |
Comment on lines
+90
to
+93
| const auto *SmartptrResetCall = | ||
| Result.Nodes.getNodeAs<CXXMemberCallExpr>("smartptrResetCall"); | ||
| const auto *ObjectResetCall = | ||
| Result.Nodes.getNodeAs<CXXMemberCallExpr>("objectResetCall"); |
There was a problem hiding this comment.
Please move these into each of the if conditions to potentially save the one lookup from objectResetCall:
if (const auto *SmartptrResetCall = Result.Nodes.getNodeAs<CXXMemberCallExpr>("smartptrResetCall")) {
...
}| DiagBuilder << FixItHint::CreateReplacement( | ||
| SourceRange(Member->getMemberLoc(), SmartptrResetCall->getEndLoc()), | ||
| " nullptr"); | ||
| } |
There was a problem hiding this comment.
At this point you can return for inside this if branch, because the other case will never happen in the same check call
| 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; |
There was a problem hiding this comment.
I disagree, the code issue this check is hitting for also relevant for C++03. Think about boost::shared_ptr, for example.
There was a problem hiding this comment.
As for now, this check only matches ::std::unique_ptr and ::std::shared_ptr, so I think this comment is valid.
In the future, I think a list of smartptr-like classes should be an option for check.
Comment on lines
+148
to
+149
| CheckFactories.registerCheck<SmartptrResetAmbiguousCallCheck>( | ||
| "bugprone-smartptr-reset-ambiguous-call"); |
There was a problem hiding this comment.
Please sort alphabetically
| if (ObjectResetCall) { | ||
| const auto *Arrow = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("OpCall"); | ||
|
|
||
| const auto SmartptrSourceRange = |
There was a problem hiding this comment.
auto should not be used here - type is not spelled explicitly.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
This pr closes issue #120908