[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
PiotrZSL wrote: @vbvictor If you don't want this to be merged as "70346889+vbvic...@users.noreply.github.com.", please change your email privacy settings. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
EugeneZelenko wrote: > @HerrCai0907, could you please address [#121291 > (comment)](https://github.com/llvm/llvm-project/pull/121291#issuecomment-2702153354). > Thank you! Can I ask someone to grant me write access to repository? Since > you basically the only one who merges PRs of other people, this could lower > amount of work you need to do. Also, I'd be happy to help new contributors in > merging their PRs. If you have at least three merged pull requests, you could request commit access using [other ones](https://github.com/llvm/llvm-project/issues/?q=is%3Aissue%20state%3Aopen%20%20label%3Ainfra%3Acommit-access-request) as example. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/PiotrZSL edited https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
github-actions[bot] wrote: @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](https://lab.llvm.org/buildbot/). 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](https://llvm.org/docs/MyFirstTypoFix.html#myfirsttypofix-issues-after-landing-your-pr). 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](https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy). 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! https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/PiotrZSL closed https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
vbvictor wrote: Ping @HerrCai0907, could you please address https://github.com/llvm/llvm-project/pull/121291#issuecomment-2702153354. Thank you! Can I ask someone to grant me write access to repository? Since you basically the only one who merges PRs of other people, this could lower amount of work you need to do. Also, I'd be happy to help new contributors in merging their PRs. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
vbvictor wrote: Since all approved are made, can we land this PR? @HerrCai0907 https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor updated https://github.com/llvm/llvm-project/pull/121291 >From 099aacc263f70301efefa722d49874a47b0054a9 Mon Sep 17 00:00:00 2001 From: Victor Baranov Date: Sat, 1 Mar 2025 18:05:54 +0300 Subject: [PATCH] add ambigous-smartptr-reset-call check --- .../AmbiguousSmartptrResetCallCheck.cpp | 126 ++ .../AmbiguousSmartptrResetCallCheck.h | 37 ++ .../clang-tidy/readability/CMakeLists.txt | 1 + .../readability/ReadabilityTidyModule.cpp | 3 + clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../docs/clang-tidy/checks/list.rst | 1 + .../ambiguous-smartptr-reset-call.rst | 62 +++ ...us-smartptr-reset-call-custom-pointers.cpp | 52 +++ .../ambiguous-smartptr-reset-call.cpp | 397 ++ 9 files changed, 685 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/readability/AmbiguousSmartptrResetCallCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/readability/AmbiguousSmartptrResetCallCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/readability/ambiguous-smartptr-reset-call.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/readability/ambiguous-smartptr-reset-call-custom-pointers.cpp create mode 100644 clang-tools-extra/test/clang-tidy/checkers/readability/ambiguous-smartptr-reset-call.cpp diff --git a/clang-tools-extra/clang-tidy/readability/AmbiguousSmartptrResetCallCheck.cpp b/clang-tools-extra/clang-tidy/readability/AmbiguousSmartptrResetCallCheck.cpp new file mode 100644 index 0..5f36c3976fc69 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/AmbiguousSmartptrResetCallCheck.cpp @@ -0,0 +1,126 @@ +//===--- AmbiguousSmartptrResetCallCheck.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 "AmbiguousSmartptrResetCallCheck.h" +#include "../utils/OptionsUtils.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::readability { + +namespace { + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr;" + "::boost::shared_ptr"; +} // namespace + +AmbiguousSmartptrResetCallCheck::AmbiguousSmartptrResetCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void AmbiguousSmartptrResetCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::serializeStringList(SmartPointers)); +} + +void AmbiguousSmartptrResetCallCheck::registerMatchers(MatchFinder *Finder) { + const auto IsSmartptr = hasAnyName(SmartPointers); + + const auto ResetMethod = + cxxMethodDecl(hasName("reset"), hasOnlyDefaultParameters()); + + const auto TypeWithReset = + anyOf(cxxRecordDecl( +anyOf(hasMethod(ResetMethod), + isDerivedFrom(cxxRecordDecl(hasMethod(ResetMethod), +classTemplateSpecializationDecl( +hasSpecializedTemplate(classTemplateDecl(has(ResetMethod); + + const auto SmartptrWithReset = expr(hasType(hasUnqualifiedDesugaredType( + recordType(hasDeclaration(classTemplateSpecializationDecl( + IsSmartptr, + hasTemplateArgument( + 0, templateArgument(refersToType(hasUnqualifiedDesugaredType( + recordType(hasDeclaration(TypeWithReset; + + Finder->addMatcher( + cxxMemberCallExpr( + callee(ResetMethod), + unless(hasAnyArgument(expr(unless(cxxDefaultArgExpr(), + anyOf(on(cxxOperatorCallExpr(hasOverloadedOperatorName("->"), + hasArgument(0, SmartptrWithReset)) + .bind("ArrowOp")), +on(SmartptrWithReset))) + .bind("MemberCall"), + this); +} + +void AmbiguousSmartptrResetCallCheck::check( +const MatchFinder::MatchResult &Result) { + const auto *MemberCall = + Result.Nodes.getNodeAs("MemberCall"); + assert(MemberCall); + + if (const auto *Arrow = + Result.Nodes.getNodeAs("ArrowOp")) { +const CharSourceRange SmartptrSourceRange = +Lexer::getAsCharRange(Arrow->getArg(0)->getSourceRange(), +
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor edited https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,129 @@ +//===--- AmbiguousSmartptrResetCallCheck.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 "AmbiguousSmartptrResetCallCheck.h" +#include "../utils/OptionsUtils.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::readability { + +namespace { + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr;" + "::boost::shared_ptr"; +} // namespace + +AmbiguousSmartptrResetCallCheck::AmbiguousSmartptrResetCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void AmbiguousSmartptrResetCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::serializeStringList(SmartPointers)); +} + +void AmbiguousSmartptrResetCallCheck::registerMatchers(MatchFinder *Finder) { + const auto IsSmartptr = hasAnyName(SmartPointers); + + const auto ResetMethod = + cxxMethodDecl(hasName("reset"), hasOnlyDefaultParameters()); + + const auto TypeWithReset = + anyOf(cxxRecordDecl(hasMethod(ResetMethod)), +classTemplateSpecializationDecl( +hasSpecializedTemplate(classTemplateDecl(has(ResetMethod); + + const auto SmartptrWithReset = expr(hasType(hasUnqualifiedDesugaredType( + recordType(hasDeclaration(classTemplateSpecializationDecl( + IsSmartptr, + hasTemplateArgument( + 0, templateArgument(refersToType(hasUnqualifiedDesugaredType( + recordType(hasDeclaration(TypeWithReset; + + Finder->addMatcher( + cxxMemberCallExpr( + callee(ResetMethod), + unless(hasAnyArgument(expr(unless(cxxDefaultArgExpr(), + anyOf(on(SmartptrWithReset), +on(cxxOperatorCallExpr(hasOverloadedOperatorName("->"), + hasArgument(0, SmartptrWithReset)) + .bind("ArrowOp" + .bind("MemberCall"), + this); +} + +void AmbiguousSmartptrResetCallCheck::check( +const MatchFinder::MatchResult &Result) { + + if (const auto *Arrow = + Result.Nodes.getNodeAs("ArrowOp")) { +const auto *ObjectResetCall = +Result.Nodes.getNodeAs("MemberCall"); + +const CharSourceRange SmartptrSourceRange = +Lexer::getAsCharRange(Arrow->getArg(0)->getSourceRange(), vbvictor wrote: Binding to variable caused test-failures, and since it isn't a "hot path" I'd prefer to leave as is. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,68 @@ +// RUN: %check_clang_tidy %s readability-ambiguous-smartptr-reset-call %t \ vbvictor wrote: I'd prefer to leave two separate files, they are easier to read and maintain imho. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor edited https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,57 @@ +.. title:: clang-tidy - readability-ambiguous-smartptr-reset-call + +readability-ambiguous-smartptr-reset-call += + +Finds potentially erroneous calls to ``reset`` method on smart pointers when +the pointee type also has a ``reset`` method. Having a ``reset`` method in +both classes 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(); + + 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 a 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 ptr = std::make_unique(); + + (*ptr).reset(); // Clearly calls underlying reset method + ptr = nullptr; // Clearly makes the pointer null + +The default smart pointers and classes that are considered are +``std::unique_ptr``, ``std::shared_ptr``, ``boost::shared_ptr``. To specify +other smart pointers or other classes use the :option:`SmartPointers` option. + + +.. note:: + + The check may emit invalid fix-its and misleading warning messages when + specifying custom smart pointers or other classes in the + :option:`SmartPointers` option. For example, ``boost::scoped_ptr`` does not + have an ``operator=`` which makes fix-its invalid. + + +Options +--- + +.. option:: SmartPointers + +Semicolon-separated list of fully qualified class names of custom smart +pointers. Default value is `::std::unique_ptr;::std::shared_ptr; +::boost::shared_ptr`. vbvictor wrote: As I said in https://github.com/llvm/llvm-project/pull/121291#issuecomment-2676086944, I think it's better to provide default pointers whose fixes will be 100% correct (hopefully). You think we should put values that provide incorrect fix-its sometimes? https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,57 @@ +.. title:: clang-tidy - readability-ambiguous-smartptr-reset-call + +readability-ambiguous-smartptr-reset-call += + +Finds potentially erroneous calls to ``reset`` method on smart pointers when +the pointee type also has a ``reset`` method. Having a ``reset`` method in +both classes 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(); + + 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 a 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 ptr = std::make_unique(); + + (*ptr).reset(); // Clearly calls underlying reset method + ptr = nullptr; // Clearly makes the pointer null + +The default smart pointers and classes that are considered are +``std::unique_ptr``, ``std::shared_ptr``, ``boost::shared_ptr``. To specify +other smart pointers or other classes use the :option:`SmartPointers` option. + + +.. note:: + + The check may emit invalid fix-its and misleading warning messages when + specifying custom smart pointers or other classes in the + :option:`SmartPointers` option. For example, ``boost::scoped_ptr`` does not + have an ``operator=`` which makes fix-its invalid. + + +Options +--- + +.. option:: SmartPointers + +Semicolon-separated list of fully qualified class names of custom smart +pointers. Default value is `::std::unique_ptr;::std::shared_ptr; +::boost::shared_ptr`. PiotrZSL wrote: That's not about fixes, it is about default settings for configuration option. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,57 @@ +.. title:: clang-tidy - readability-ambiguous-smartptr-reset-call + +readability-ambiguous-smartptr-reset-call += + +Finds potentially erroneous calls to ``reset`` method on smart pointers when +the pointee type also has a ``reset`` method. Having a ``reset`` method in +both classes 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(); + + 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 a 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 ptr = std::make_unique(); + + (*ptr).reset(); // Clearly calls underlying reset method + ptr = nullptr; // Clearly makes the pointer null + +The default smart pointers and classes that are considered are +``std::unique_ptr``, ``std::shared_ptr``, ``boost::shared_ptr``. To specify +other smart pointers or other classes use the :option:`SmartPointers` option. + + +.. note:: + + The check may emit invalid fix-its and misleading warning messages when + specifying custom smart pointers or other classes in the + :option:`SmartPointers` option. For example, ``boost::scoped_ptr`` does not + have an ``operator=`` which makes fix-its invalid. + + +Options +--- + +.. option:: SmartPointers + +Semicolon-separated list of fully qualified class names of custom smart +pointers. Default value is `::std::unique_ptr;::std::shared_ptr; +::boost::shared_ptr`. vbvictor wrote: I think here my code is correct since `::boost::scoped_ptr` does not have `operator=` which causes fix-its to be invalid. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,68 @@ +// RUN: %check_clang_tidy %s readability-ambiguous-smartptr-reset-call %t \ PiotrZSL wrote: 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) https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,129 @@ +//===--- AmbiguousSmartptrResetCallCheck.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 "AmbiguousSmartptrResetCallCheck.h" +#include "../utils/OptionsUtils.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::readability { + +namespace { + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr;" + "::boost::shared_ptr"; +} // namespace + +AmbiguousSmartptrResetCallCheck::AmbiguousSmartptrResetCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void AmbiguousSmartptrResetCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::serializeStringList(SmartPointers)); +} + +void AmbiguousSmartptrResetCallCheck::registerMatchers(MatchFinder *Finder) { + const auto IsSmartptr = hasAnyName(SmartPointers); + + const auto ResetMethod = + cxxMethodDecl(hasName("reset"), hasOnlyDefaultParameters()); + + const auto TypeWithReset = + anyOf(cxxRecordDecl(hasMethod(ResetMethod)), +classTemplateSpecializationDecl( +hasSpecializedTemplate(classTemplateDecl(has(ResetMethod); + + const auto SmartptrWithReset = expr(hasType(hasUnqualifiedDesugaredType( + recordType(hasDeclaration(classTemplateSpecializationDecl( + IsSmartptr, + hasTemplateArgument( + 0, templateArgument(refersToType(hasUnqualifiedDesugaredType( + recordType(hasDeclaration(TypeWithReset; + + Finder->addMatcher( + cxxMemberCallExpr( + callee(ResetMethod), + unless(hasAnyArgument(expr(unless(cxxDefaultArgExpr(), + anyOf(on(SmartptrWithReset), +on(cxxOperatorCallExpr(hasOverloadedOperatorName("->"), + hasArgument(0, SmartptrWithReset)) + .bind("ArrowOp" + .bind("MemberCall"), + this); +} + +void AmbiguousSmartptrResetCallCheck::check( +const MatchFinder::MatchResult &Result) { + + if (const auto *Arrow = + Result.Nodes.getNodeAs("ArrowOp")) { +const auto *ObjectResetCall = +Result.Nodes.getNodeAs("MemberCall"); PiotrZSL wrote: duplicated member call, just move it before if and use in both if's https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,57 @@ +.. title:: clang-tidy - readability-ambiguous-smartptr-reset-call + +readability-ambiguous-smartptr-reset-call += + +Finds potentially erroneous calls to ``reset`` method on smart pointers when +the pointee type also has a ``reset`` method. Having a ``reset`` method in +both classes 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(); + + 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 a 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 ptr = std::make_unique(); + + (*ptr).reset(); // Clearly calls underlying reset method + ptr = nullptr; // Clearly makes the pointer null + +The default smart pointers and classes that are considered are +``std::unique_ptr``, ``std::shared_ptr``, ``boost::shared_ptr``. To specify +other smart pointers or other classes use the :option:`SmartPointers` option. + + PiotrZSL wrote: Mention in documentation that fixes are enabled via `--fix-notes` https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,57 @@ +.. title:: clang-tidy - readability-ambiguous-smartptr-reset-call + +readability-ambiguous-smartptr-reset-call += + +Finds potentially erroneous calls to ``reset`` method on smart pointers when +the pointee type also has a ``reset`` method. Having a ``reset`` method in +both classes 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(); + + 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 a 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 ptr = std::make_unique(); + + (*ptr).reset(); // Clearly calls underlying reset method + ptr = nullptr; // Clearly makes the pointer null + +The default smart pointers and classes that are considered are +``std::unique_ptr``, ``std::shared_ptr``, ``boost::shared_ptr``. To specify +other smart pointers or other classes use the :option:`SmartPointers` option. + + +.. note:: + + The check may emit invalid fix-its and misleading warning messages when + specifying custom smart pointers or other classes in the + :option:`SmartPointers` option. For example, ``boost::scoped_ptr`` does not + have an ``operator=`` which makes fix-its invalid. + + +Options +--- + +.. option:: SmartPointers + +Semicolon-separated list of fully qualified class names of custom smart +pointers. Default value is `::std::unique_ptr;::std::shared_ptr; +::boost::shared_ptr`. PiotrZSL wrote: ```suggestion ::boost::scoped_ptr`. ``` https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/PiotrZSL approved this pull request. In general looks fine. Few nits. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/PiotrZSL edited https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,129 @@ +//===--- AmbiguousSmartptrResetCallCheck.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 "AmbiguousSmartptrResetCallCheck.h" +#include "../utils/OptionsUtils.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::readability { + +namespace { + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr;" + "::boost::shared_ptr"; +} // namespace + +AmbiguousSmartptrResetCallCheck::AmbiguousSmartptrResetCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void AmbiguousSmartptrResetCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::serializeStringList(SmartPointers)); +} + +void AmbiguousSmartptrResetCallCheck::registerMatchers(MatchFinder *Finder) { + const auto IsSmartptr = hasAnyName(SmartPointers); + + const auto ResetMethod = + cxxMethodDecl(hasName("reset"), hasOnlyDefaultParameters()); + + const auto TypeWithReset = + anyOf(cxxRecordDecl(hasMethod(ResetMethod)), +classTemplateSpecializationDecl( +hasSpecializedTemplate(classTemplateDecl(has(ResetMethod); + + const auto SmartptrWithReset = expr(hasType(hasUnqualifiedDesugaredType( + recordType(hasDeclaration(classTemplateSpecializationDecl( + IsSmartptr, + hasTemplateArgument( + 0, templateArgument(refersToType(hasUnqualifiedDesugaredType( + recordType(hasDeclaration(TypeWithReset; + + Finder->addMatcher( + cxxMemberCallExpr( + callee(ResetMethod), + unless(hasAnyArgument(expr(unless(cxxDefaultArgExpr(), + anyOf(on(SmartptrWithReset), +on(cxxOperatorCallExpr(hasOverloadedOperatorName("->"), + hasArgument(0, SmartptrWithReset)) + .bind("ArrowOp" + .bind("MemberCall"), + this); +} + +void AmbiguousSmartptrResetCallCheck::check( +const MatchFinder::MatchResult &Result) { + + if (const auto *Arrow = + Result.Nodes.getNodeAs("ArrowOp")) { +const auto *ObjectResetCall = +Result.Nodes.getNodeAs("MemberCall"); + +const CharSourceRange SmartptrSourceRange = +Lexer::getAsCharRange(Arrow->getArg(0)->getSourceRange(), PiotrZSL wrote: maybe just 'bind' an object to some variable and read it like an MemberCall https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,68 @@ +// RUN: %check_clang_tidy %s readability-ambiguous-smartptr-reset-call %t \ +// RUN: -config='{CheckOptions: \ +// RUN: {readability-ambiguous-smartptr-reset-call.SmartPointers: "::std::unique_ptr;::other_ptr"}}' \ +// RUN: --fix-notes -- + +namespace std { + +template PiotrZSL wrote: consider including clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/smart-ptr/shared_ptr.h and clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/smart-ptr/unique_ptr.h instead https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/denzor200 approved this pull request. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
vbvictor wrote: @PiotrZSL, @denzor200 I think we should provide 100% valid fix-its for default `SmartPointers`. That will be `std::shared_ptr`, `std::unique_ptr`, `boost::shared_ptr`. I created a note in docs that warns users about inconvenience that can happen when non-default smart pointers are specified. May you please review one more time. If everything is good i'd need help in merging this pr since i don't have write access to main https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor updated https://github.com/llvm/llvm-project/pull/121291 >From 42e03bb9cc9bd815476b0a3f06ac5f58826e3708 Mon Sep 17 00:00:00 2001 From: Victor Baranov Date: Fri, 31 Jan 2025 19:29:05 +0300 Subject: [PATCH 1/8] [clang-tidy] add new check bugprone-reset-ambiguous-call --- .../bugprone/BugproneTidyModule.cpp | 3 + .../clang-tidy/bugprone/CMakeLists.txt| 1 + .../SmartptrResetAmbiguousCallCheck.cpp | 146 +++ .../SmartptrResetAmbiguousCallCheck.h | 37 +++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../smartptr-reset-ambiguous-call.rst | 47 .../docs/clang-tidy/checks/list.rst | 1 + ...r-reset-ambiguous-call-custom-pointers.cpp | 72 ++ .../smartptr-reset-ambiguous-call.cpp | 239 ++ 9 files changed, 552 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/smartptr-reset-ambiguous-call.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/smartptr-reset-ambiguous-call-custom-pointers.cpp create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/smartptr-reset-ambiguous-call.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index c5f0b5b28418f..a01d0e384ab73 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -64,6 +64,7 @@ #include "SignedCharMisuseCheck.h" #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" +#include "SmartptrResetAmbiguousCallCheck.h" #include "SpuriouslyWakeUpFunctionsCheck.h" #include "StandaloneEmptyCheck.h" #include "StringConstructorCheck.h" @@ -207,6 +208,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-sizeof-container"); CheckFactories.registerCheck( "bugprone-sizeof-expression"); +CheckFactories.registerCheck( +"bugprone-smartptr-reset-ambiguous-call"); CheckFactories.registerCheck( "bugprone-spuriously-wake-up-functions"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index e8309c68b7fca..7fd6f4994f537 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -65,6 +65,7 @@ add_clang_library(clangTidyBugproneModule STATIC SizeofContainerCheck.cpp SizeofExpressionCheck.cpp SmartPtrArrayMismatchCheck.cpp + SmartptrResetAmbiguousCallCheck.cpp SpuriouslyWakeUpFunctionsCheck.cpp StandaloneEmptyCheck.cpp StringConstructorCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp new file mode 100644 index 0..326a5665179d7 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp @@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::seriali
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,130 @@ +//===--- AmbiguousSmartptrResetCallCheck.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 "AmbiguousSmartptrResetCallCheck.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Lex/Lexer.h" +#include + +using namespace clang::ast_matchers; + +namespace clang::tidy::readability { + +namespace { + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr;" + "::boost::shared_ptr;::boost::scoped_ptr"; vbvictor wrote: I think we should stick to 100% correct fix-its for default-defined types and warn users about incorrect behavoir of other types that lack `operator=` or `operator=` doesn't expect `std::nullptr_t`. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,130 @@ +//===--- AmbiguousSmartptrResetCallCheck.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 "AmbiguousSmartptrResetCallCheck.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Lex/Lexer.h" +#include + +using namespace clang::ast_matchers; + +namespace clang::tidy::readability { + +namespace { + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr;" + "::boost::shared_ptr;::boost::scoped_ptr"; vbvictor wrote: Thank you for pointing that out. I have written a note in documentation that says the check may not provide correct fix-its on user-defined types. I guess I will also include `::boost::scoped_ptr` as an example. As for `::boost::scoped_ptr` the correct fix-it would be `ptr.reset(nullptr)` or `ptr.reset(0)` https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,130 @@ +//===--- AmbiguousSmartptrResetCallCheck.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 "AmbiguousSmartptrResetCallCheck.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Lex/Lexer.h" +#include + +using namespace clang::ast_matchers; + +namespace clang::tidy::readability { + +namespace { + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr;" + "::boost::shared_ptr;::boost::scoped_ptr"; denzor200 wrote: Since this check is unable to handle the issue with `::boost::scoped_ptr`, here I requested another check to get rid of scoped_ptr: https://github.com/llvm/llvm-project/issues/128179 https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/denzor200 requested changes to this pull request. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,130 @@ +//===--- AmbiguousSmartptrResetCallCheck.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 "AmbiguousSmartptrResetCallCheck.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Lex/Lexer.h" +#include + +using namespace clang::ast_matchers; + +namespace clang::tidy::readability { + +namespace { + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr;" + "::boost::shared_ptr;::boost::scoped_ptr"; denzor200 wrote: yet another one wrong fix hint, `::boost::scoped_ptr` doesn't have `operator=` at all :( No way to reset this pointer without `reset`, except the crutch like calling `swap` method with a reference to default initialized object passed. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor updated https://github.com/llvm/llvm-project/pull/121291 >From 42e03bb9cc9bd815476b0a3f06ac5f58826e3708 Mon Sep 17 00:00:00 2001 From: Victor Baranov Date: Fri, 31 Jan 2025 19:29:05 +0300 Subject: [PATCH 1/7] [clang-tidy] add new check bugprone-reset-ambiguous-call --- .../bugprone/BugproneTidyModule.cpp | 3 + .../clang-tidy/bugprone/CMakeLists.txt| 1 + .../SmartptrResetAmbiguousCallCheck.cpp | 146 +++ .../SmartptrResetAmbiguousCallCheck.h | 37 +++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../smartptr-reset-ambiguous-call.rst | 47 .../docs/clang-tidy/checks/list.rst | 1 + ...r-reset-ambiguous-call-custom-pointers.cpp | 72 ++ .../smartptr-reset-ambiguous-call.cpp | 239 ++ 9 files changed, 552 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/smartptr-reset-ambiguous-call.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/smartptr-reset-ambiguous-call-custom-pointers.cpp create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/smartptr-reset-ambiguous-call.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index c5f0b5b28418f..a01d0e384ab73 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -64,6 +64,7 @@ #include "SignedCharMisuseCheck.h" #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" +#include "SmartptrResetAmbiguousCallCheck.h" #include "SpuriouslyWakeUpFunctionsCheck.h" #include "StandaloneEmptyCheck.h" #include "StringConstructorCheck.h" @@ -207,6 +208,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-sizeof-container"); CheckFactories.registerCheck( "bugprone-sizeof-expression"); +CheckFactories.registerCheck( +"bugprone-smartptr-reset-ambiguous-call"); CheckFactories.registerCheck( "bugprone-spuriously-wake-up-functions"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index e8309c68b7fca..7fd6f4994f537 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -65,6 +65,7 @@ add_clang_library(clangTidyBugproneModule STATIC SizeofContainerCheck.cpp SizeofExpressionCheck.cpp SmartPtrArrayMismatchCheck.cpp + SmartptrResetAmbiguousCallCheck.cpp SpuriouslyWakeUpFunctionsCheck.cpp StandaloneEmptyCheck.cpp StringConstructorCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp new file mode 100644 index 0..326a5665179d7 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp @@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::seriali
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,49 @@ +.. title:: clang-tidy - readability-ambiguous-smartptr-reset-call + +readability-ambiguous-smartptr-reset-call += + +Finds potentially erroneous calls to ``reset`` method on smart pointers when +the pointee type also has a ``reset`` method. Having a ``reset`` method in +both classes 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(); + + 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 a 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 ptr = std::make_unique(); + + (*ptr).reset(); // Clearly calls underlying reset method + ptr = nullptr; // Clearly makes the pointer null + +The default smart pointers and classes that are considered are +``std::unique_ptr``, ``std::shared_ptr``, ``std::optional``, +``boost::shared_ptr``, ``boost::scoped_ptr``. To specify other smart pointers +or other classes use the :option:`SmartPointers` option. + +Options +--- + +.. option:: SmartPointers + +Semicolon-separated list of fully qualified class names of custom smart +pointers. Default value is `::std::unique_ptr;::std::shared_ptr; +::std::optional;::boost::unique_ptr;::boost::shared_ptr`. vbvictor wrote: Done https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor updated https://github.com/llvm/llvm-project/pull/121291 >From 42e03bb9cc9bd815476b0a3f06ac5f58826e3708 Mon Sep 17 00:00:00 2001 From: Victor Baranov Date: Fri, 31 Jan 2025 19:29:05 +0300 Subject: [PATCH 1/6] [clang-tidy] add new check bugprone-reset-ambiguous-call --- .../bugprone/BugproneTidyModule.cpp | 3 + .../clang-tidy/bugprone/CMakeLists.txt| 1 + .../SmartptrResetAmbiguousCallCheck.cpp | 146 +++ .../SmartptrResetAmbiguousCallCheck.h | 37 +++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../smartptr-reset-ambiguous-call.rst | 47 .../docs/clang-tidy/checks/list.rst | 1 + ...r-reset-ambiguous-call-custom-pointers.cpp | 72 ++ .../smartptr-reset-ambiguous-call.cpp | 239 ++ 9 files changed, 552 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/smartptr-reset-ambiguous-call.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/smartptr-reset-ambiguous-call-custom-pointers.cpp create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/smartptr-reset-ambiguous-call.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index c5f0b5b28418f..a01d0e384ab73 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -64,6 +64,7 @@ #include "SignedCharMisuseCheck.h" #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" +#include "SmartptrResetAmbiguousCallCheck.h" #include "SpuriouslyWakeUpFunctionsCheck.h" #include "StandaloneEmptyCheck.h" #include "StringConstructorCheck.h" @@ -207,6 +208,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-sizeof-container"); CheckFactories.registerCheck( "bugprone-sizeof-expression"); +CheckFactories.registerCheck( +"bugprone-smartptr-reset-ambiguous-call"); CheckFactories.registerCheck( "bugprone-spuriously-wake-up-functions"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index e8309c68b7fca..7fd6f4994f537 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -65,6 +65,7 @@ add_clang_library(clangTidyBugproneModule STATIC SizeofContainerCheck.cpp SizeofExpressionCheck.cpp SmartPtrArrayMismatchCheck.cpp + SmartptrResetAmbiguousCallCheck.cpp SpuriouslyWakeUpFunctionsCheck.cpp StandaloneEmptyCheck.cpp StringConstructorCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp new file mode 100644 index 0..326a5665179d7 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp @@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::seriali
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,131 @@ +//===--- AmbiguousSmartptrResetCallCheck.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 "AmbiguousSmartptrResetCallCheck.h" +#include "../utils/OptionsUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Lex/Lexer.h" +#include + +using namespace clang::ast_matchers; + +namespace clang::tidy::readability { + +namespace { + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = +"::std::shared_ptr;::std::unique_ptr;::std::optional;" denzor200 wrote: remove `std::optional` from this check https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/denzor200 requested changes to this pull request. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,48 @@ +.. title:: clang-tidy - readability-ambiguous-smartptr-reset-call + +readability-ambiguous-smartptr-reset-call += + +Finds potentially erroneous calls to ``reset`` method on smart pointers when +the pointee type also has a ``reset`` method. Having a ``reset`` method in +both classes 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(); + + 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 a 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 ptr = std::make_unique(); + + (*ptr).reset(); // Clearly calls underlying reset method + ptr = nullptr; // Clearly makes the pointer null + +The default smart pointers that are considered are ``std::unique_ptr``, +``std::shared_ptr``, ``boost::unique_ptr``, ``boost::shared_ptr``. To specify +other smart pointers or other classes use the :option:`SmartPointers` option. + +Options +--- + +.. option:: SmartPointers + +Semicolon-separated list of fully qualified class names of custom smart +pointers. Default value is `::std::unique_ptr;::std::shared_ptr; denzor200 wrote: > consider adding std::optional here also (even that is not smart ptr) Strictly disagree with you, optional-like classes are not assignable with `nullptr`. Here is no place for them in this check otherwise it will produce wrong fix hints: ``` { std::optional o; o.reset(); // should produce `o = std::nullopt;` but `o = nullptr;` does } { boost::optional o; o.reset(); // should produce `o = boost::none;` but `o = nullptr;` does } ``` https://godbolt.org/z/YM79G8zdP https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor edited https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::serializeStringList(SmartPointers)); +} + +void SmartptrResetAmbiguousCallCheck::registerMatchers(MatchFinder *Finder) { + const auto IsSmartptr = hasAnyName(SmartPointers); + + const auto ResetMethod = + cxxMethodDecl(hasName("reset"), hasOnlyDefaultParameters()); + + 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( vbvictor wrote: With some modifications made this work, changed `thisPointerType()` to `on(expr())` because it failed to match some cases with nested ptr's. `std::unique_ptr>> nested_ptr;` https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,48 @@ +.. title:: clang-tidy - readability-ambiguous-smartptr-reset-call + +readability-ambiguous-smartptr-reset-call += + +Finds potentially erroneous calls to ``reset`` method on smart pointers when +the pointee type also has a ``reset`` method. Having a ``reset`` method in +both classes 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(); + + 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 a 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 ptr = std::make_unique(); + + (*ptr).reset(); // Clearly calls underlying reset method + ptr = nullptr; // Clearly makes the pointer null + +The default smart pointers that are considered are ``std::unique_ptr``, +``std::shared_ptr``, ``boost::unique_ptr``, ``boost::shared_ptr``. To specify +other smart pointers or other classes use the :option:`SmartPointers` option. + +Options +--- + +.. option:: SmartPointers + +Semicolon-separated list of fully qualified class names of custom smart +pointers. Default value is `::std::unique_ptr;::std::shared_ptr; vbvictor wrote: Synced code and docs, sorry for inconvenience. Also added tests to cover all listed types. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,48 @@ +.. title:: clang-tidy - readability-ambiguous-smartptr-reset-call + +readability-ambiguous-smartptr-reset-call += + +Finds potentially erroneous calls to ``reset`` method on smart pointers when +the pointee type also has a ``reset`` method. Having a ``reset`` method in +both classes 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(); + + 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 a 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 ptr = std::make_unique(); + + (*ptr).reset(); // Clearly calls underlying reset method + ptr = nullptr; // Clearly makes the pointer null + +The default smart pointers that are considered are ``std::unique_ptr``, +``std::shared_ptr``, ``boost::unique_ptr``, ``boost::shared_ptr``. To specify vbvictor wrote: Fixed https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,239 @@ +// RUN: %check_clang_tidy %s readability-ambiguous-smartptr-reset-call %t --fix-notes -- + +namespace std { + +template +struct unique_ptr { + T& operator*() const; + T* operator->() const; + void reset(T* p = nullptr); +}; + +template +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 u; + u.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a smart pointer with pointee that also has a 'reset()' method, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider assigning the pointer to 'nullptr' here + // CHECK-FIXES: u = nullptr; + u->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a pointee of a smart pointer, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider dereferencing smart pointer to call 'reset' method of the pointee here + // CHECK-FIXES: (*u).reset(); + + std::shared_ptr s; + s.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a smart pointer with pointee that also has a 'reset()' method, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider assigning the pointer to 'nullptr' here + // CHECK-FIXES: s = nullptr; + s->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a pointee of a smart pointer, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider dereferencing smart pointer to call 'reset' method of the pointee here + // CHECK-FIXES: (*s).reset(); + + std::unique_ptr> uu_ptr; + uu_ptr.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a smart pointer with pointee that also has a 'reset()' method, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider assigning the pointer to 'nullptr' here + // CHECK-FIXES: uu_ptr = nullptr; + uu_ptr->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a pointee of a smart pointer, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider dereferencing smart pointer to call 'reset' method of the pointee here + // CHECK-FIXES: (*uu_ptr).reset(); + + std::unique_ptr> su_ptr; + su_ptr.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a smart pointer with pointee that also has a 'reset()' method, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider assigning the pointer to 'nullptr' here + // CHECK-FIXES: su_ptr = nullptr; + su_ptr->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a pointee of a smart pointer, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider dereferencing smart pointer to call 'reset' method of the pointee here + // CHECK-FIXES: (*su_ptr).reset(); + + std::unique_ptr rd; + rd.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a smart pointer with pointee that also has a 'reset()' method, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider assigning the pointer to 'nullptr' here + // CHECK-FIXES: rd = nullptr; + rd->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a pointee of a smart pointer, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider dereferencing smart pointer to call 'reset' method of the pointee here + // CHECK-FIXES: (*rd).reset(); + + std::unique_ptr>> nested_ptr; + nested_ptr.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a smart pointer with pointee that also has a 'reset()' method, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider assigning the pointer to 'nullptr' here + // CHECK-FIXES: nested_ptr = nullptr; + nested_ptr->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a pointee of a smart pointer, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider dereferencing smart pointer to call 'reset' method of the pointee here + // CHECK-FIXES: (*nested_ptr).reset(); + (*nested_ptr).reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a smart pointer with pointee that also has a 'reset()' method, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider assigni
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor updated https://github.com/llvm/llvm-project/pull/121291 >From 42e03bb9cc9bd815476b0a3f06ac5f58826e3708 Mon Sep 17 00:00:00 2001 From: Victor Baranov Date: Fri, 31 Jan 2025 19:29:05 +0300 Subject: [PATCH 1/5] [clang-tidy] add new check bugprone-reset-ambiguous-call --- .../bugprone/BugproneTidyModule.cpp | 3 + .../clang-tidy/bugprone/CMakeLists.txt| 1 + .../SmartptrResetAmbiguousCallCheck.cpp | 146 +++ .../SmartptrResetAmbiguousCallCheck.h | 37 +++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../smartptr-reset-ambiguous-call.rst | 47 .../docs/clang-tidy/checks/list.rst | 1 + ...r-reset-ambiguous-call-custom-pointers.cpp | 72 ++ .../smartptr-reset-ambiguous-call.cpp | 239 ++ 9 files changed, 552 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/smartptr-reset-ambiguous-call.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/smartptr-reset-ambiguous-call-custom-pointers.cpp create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/smartptr-reset-ambiguous-call.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index c5f0b5b28418f..a01d0e384ab73 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -64,6 +64,7 @@ #include "SignedCharMisuseCheck.h" #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" +#include "SmartptrResetAmbiguousCallCheck.h" #include "SpuriouslyWakeUpFunctionsCheck.h" #include "StandaloneEmptyCheck.h" #include "StringConstructorCheck.h" @@ -207,6 +208,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-sizeof-container"); CheckFactories.registerCheck( "bugprone-sizeof-expression"); +CheckFactories.registerCheck( +"bugprone-smartptr-reset-ambiguous-call"); CheckFactories.registerCheck( "bugprone-spuriously-wake-up-functions"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index e8309c68b7fca..7fd6f4994f537 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -65,6 +65,7 @@ add_clang_library(clangTidyBugproneModule STATIC SizeofContainerCheck.cpp SizeofExpressionCheck.cpp SmartPtrArrayMismatchCheck.cpp + SmartptrResetAmbiguousCallCheck.cpp SpuriouslyWakeUpFunctionsCheck.cpp StandaloneEmptyCheck.cpp StringConstructorCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp new file mode 100644 index 0..326a5665179d7 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp @@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::seriali
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor updated https://github.com/llvm/llvm-project/pull/121291 >From 42e03bb9cc9bd815476b0a3f06ac5f58826e3708 Mon Sep 17 00:00:00 2001 From: Victor Baranov Date: Fri, 31 Jan 2025 19:29:05 +0300 Subject: [PATCH 1/4] [clang-tidy] add new check bugprone-reset-ambiguous-call --- .../bugprone/BugproneTidyModule.cpp | 3 + .../clang-tidy/bugprone/CMakeLists.txt| 1 + .../SmartptrResetAmbiguousCallCheck.cpp | 146 +++ .../SmartptrResetAmbiguousCallCheck.h | 37 +++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../smartptr-reset-ambiguous-call.rst | 47 .../docs/clang-tidy/checks/list.rst | 1 + ...r-reset-ambiguous-call-custom-pointers.cpp | 72 ++ .../smartptr-reset-ambiguous-call.cpp | 239 ++ 9 files changed, 552 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/smartptr-reset-ambiguous-call.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/smartptr-reset-ambiguous-call-custom-pointers.cpp create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/smartptr-reset-ambiguous-call.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index c5f0b5b28418f..a01d0e384ab73 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -64,6 +64,7 @@ #include "SignedCharMisuseCheck.h" #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" +#include "SmartptrResetAmbiguousCallCheck.h" #include "SpuriouslyWakeUpFunctionsCheck.h" #include "StandaloneEmptyCheck.h" #include "StringConstructorCheck.h" @@ -207,6 +208,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-sizeof-container"); CheckFactories.registerCheck( "bugprone-sizeof-expression"); +CheckFactories.registerCheck( +"bugprone-smartptr-reset-ambiguous-call"); CheckFactories.registerCheck( "bugprone-spuriously-wake-up-functions"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index e8309c68b7fca..7fd6f4994f537 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -65,6 +65,7 @@ add_clang_library(clangTidyBugproneModule STATIC SizeofContainerCheck.cpp SizeofExpressionCheck.cpp SmartPtrArrayMismatchCheck.cpp + SmartptrResetAmbiguousCallCheck.cpp SpuriouslyWakeUpFunctionsCheck.cpp StandaloneEmptyCheck.cpp StringConstructorCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp new file mode 100644 index 0..326a5665179d7 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp @@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::seriali
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,48 @@ +.. title:: clang-tidy - readability-ambiguous-smartptr-reset-call + +readability-ambiguous-smartptr-reset-call += + +Finds potentially erroneous calls to ``reset`` method on smart pointers when +the pointee type also has a ``reset`` method. Having a ``reset`` method in +both classes 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(); + + 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 a 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 ptr = std::make_unique(); + + (*ptr).reset(); // Clearly calls underlying reset method + ptr = nullptr; // Clearly makes the pointer null + +The default smart pointers that are considered are ``std::unique_ptr``, +``std::shared_ptr``, ``boost::unique_ptr``, ``boost::shared_ptr``. To specify PiotrZSL wrote: boost does not have unique_ptr, it has boost::scoped_ptr https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,239 @@ +// RUN: %check_clang_tidy %s readability-ambiguous-smartptr-reset-call %t --fix-notes -- + +namespace std { + +template +struct unique_ptr { + T& operator*() const; + T* operator->() const; + void reset(T* p = nullptr); +}; + +template +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 u; + u.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a smart pointer with pointee that also has a 'reset()' method, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider assigning the pointer to 'nullptr' here + // CHECK-FIXES: u = nullptr; + u->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a pointee of a smart pointer, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider dereferencing smart pointer to call 'reset' method of the pointee here + // CHECK-FIXES: (*u).reset(); + + std::shared_ptr s; + s.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a smart pointer with pointee that also has a 'reset()' method, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider assigning the pointer to 'nullptr' here + // CHECK-FIXES: s = nullptr; + s->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a pointee of a smart pointer, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider dereferencing smart pointer to call 'reset' method of the pointee here + // CHECK-FIXES: (*s).reset(); + + std::unique_ptr> uu_ptr; + uu_ptr.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a smart pointer with pointee that also has a 'reset()' method, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider assigning the pointer to 'nullptr' here + // CHECK-FIXES: uu_ptr = nullptr; + uu_ptr->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a pointee of a smart pointer, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider dereferencing smart pointer to call 'reset' method of the pointee here + // CHECK-FIXES: (*uu_ptr).reset(); + + std::unique_ptr> su_ptr; + su_ptr.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a smart pointer with pointee that also has a 'reset()' method, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider assigning the pointer to 'nullptr' here + // CHECK-FIXES: su_ptr = nullptr; + su_ptr->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a pointee of a smart pointer, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider dereferencing smart pointer to call 'reset' method of the pointee here + // CHECK-FIXES: (*su_ptr).reset(); + + std::unique_ptr rd; + rd.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a smart pointer with pointee that also has a 'reset()' method, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider assigning the pointer to 'nullptr' here + // CHECK-FIXES: rd = nullptr; + rd->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a pointee of a smart pointer, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider dereferencing smart pointer to call 'reset' method of the pointee here + // CHECK-FIXES: (*rd).reset(); + + std::unique_ptr>> nested_ptr; + nested_ptr.reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a smart pointer with pointee that also has a 'reset()' method, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider assigning the pointer to 'nullptr' here + // CHECK-FIXES: nested_ptr = nullptr; + nested_ptr->reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a pointee of a smart pointer, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider dereferencing smart pointer to call 'reset' method of the pointee here + // CHECK-FIXES: (*nested_ptr).reset(); + (*nested_ptr).reset(); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: ambiguous call to 'reset()' on a smart pointer with pointee that also has a 'reset()' method, prefer more explicit approach + // CHECK-MESSAGES: :[[@LINE-2]]:3: note: consider assigni
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,48 @@ +.. title:: clang-tidy - readability-ambiguous-smartptr-reset-call + +readability-ambiguous-smartptr-reset-call += + +Finds potentially erroneous calls to ``reset`` method on smart pointers when +the pointee type also has a ``reset`` method. Having a ``reset`` method in +both classes 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(); + + 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 a 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 ptr = std::make_unique(); + + (*ptr).reset(); // Clearly calls underlying reset method + ptr = nullptr; // Clearly makes the pointer null + +The default smart pointers that are considered are ``std::unique_ptr``, +``std::shared_ptr``, ``boost::unique_ptr``, ``boost::shared_ptr``. To specify +other smart pointers or other classes use the :option:`SmartPointers` option. + +Options +--- + +.. option:: SmartPointers + +Semicolon-separated list of fully qualified class names of custom smart +pointers. Default value is `::std::unique_ptr;::std::shared_ptr; PiotrZSL wrote: out of sync, change also DefaultSmartPointers to match this, consider adding std::optional here also (even that is not smart ptr) https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::serializeStringList(SmartPointers)); +} + +void SmartptrResetAmbiguousCallCheck::registerMatchers(MatchFinder *Finder) { + const auto IsSmartptr = hasAnyName(SmartPointers); + + const auto ResetMethod = + cxxMethodDecl(hasName("reset"), hasOnlyDefaultParameters()); + + 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( PiotrZSL wrote: 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); ``` https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
vbvictor wrote: @PiotrZSL, thank you for pointing out better naming pattern for diag-messages, would use it in the future. 1. Created new warning and fix-it messages. 2. Renamed check. 3. As for now, my check can support any type that will have `reset` method, this include `std::optional`. Maybe a more general approach would be to detect overlapping methods with classes that are returned via `operator*` and `operator->`. However, if some method is called on main object and there is no equivalent of `= nullptr` I don't know how user can fix such warning. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::serializeStringList(SmartPointers)); +} + +void SmartptrResetAmbiguousCallCheck::registerMatchers(MatchFinder *Finder) { + const auto IsSmartptr = hasAnyName(SmartPointers); + + const auto ResetMethod = + cxxMethodDecl(hasName("reset"), hasOnlyDefaultParameters()); + + 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 SmartptrResetAmbiguousCallCheck::check( +const MatchFinder::MatchResult &Result) { + + if (const auto *SmartptrResetCall = + Result.Nodes.getNodeAs("smartptrResetCall")) { +const auto *Member = cast(SmartptrResetCall->getCallee()); + +diag(SmartptrResetCall->getBeginLoc(), + "be explicit when calling 'reset()' on a smart pointer with a " + "pointee that has a 'reset()' method"); + +diag(SmartptrResetCall->getBeginLoc(), "assign the pointer to 'nullptr'", + DiagnosticIDs::Note) +<< FixItHint::CreateReplacement( + SourceRange(Member->getOperatorLoc(), + Member->getOperatorLoc().getLocWithOffset(0)), + " =") +<< FixItHint::CreateReplacement( + SourceRange(Member->getMemberLoc(), + SmartptrResetCall->getEndLoc()), + " nullptr"); +return; + } + + if (const auto *ObjectResetCall = + Result.Nodes.getNodeAs("objectResetCall")) { +const auto *Arrow = Result.Nodes.getNodeAs("OpCall"); + +const CharSourceRange SmartptrSourceRange = +Lexer::getAsCharRange(Arrow->getArg(0)->getSourceRange(), + *Result.SourceManager, getLangOpts()); + +diag(ObjectResetCall->getBeginLoc(), + "be explicit when calling 'reset()' on a pointee of a smart pointer"); vbvictor wrote: Done https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,47 @@ +.. title:: clang-tidy - bugprone-smartptr-reset-ambiguous-call + +bugprone-smartptr-reset-ambiguous-call +== + +Finds potentially erroneous calls to ``reset`` method on smart pointers when +the pointee type also has a ``reset`` method. Having a ``reset`` method in +both classes 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(); + + 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 a 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 ptr = std::make_unique(); + + (*ptr).reset(); // Clearly calls underlying reset method + ptr = nullptr; // Clearly makes the pointer null + +The default smart pointers that are considered are ``std::unique_ptr``, +``std::shared_ptr``. To specify other smart pointers or other classes use the +:option:`SmartPointers` option. + +Options +--- + +.. option:: SmartPointers + +Semicolon-separated list of class names of custom smart pointers. vbvictor wrote: Done https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::serializeStringList(SmartPointers)); +} + +void SmartptrResetAmbiguousCallCheck::registerMatchers(MatchFinder *Finder) { + const auto IsSmartptr = hasAnyName(SmartPointers); + + const auto ResetMethod = + cxxMethodDecl(hasName("reset"), hasOnlyDefaultParameters()); + + 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 SmartptrResetAmbiguousCallCheck::check( +const MatchFinder::MatchResult &Result) { + + if (const auto *SmartptrResetCall = + Result.Nodes.getNodeAs("smartptrResetCall")) { +const auto *Member = cast(SmartptrResetCall->getCallee()); + +diag(SmartptrResetCall->getBeginLoc(), + "be explicit when calling 'reset()' on a smart pointer with a " + "pointee that has a 'reset()' method"); + +diag(SmartptrResetCall->getBeginLoc(), "assign the pointer to 'nullptr'", vbvictor wrote: Done https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::serializeStringList(SmartPointers)); +} + +void SmartptrResetAmbiguousCallCheck::registerMatchers(MatchFinder *Finder) { + const auto IsSmartptr = hasAnyName(SmartPointers); + + const auto ResetMethod = + cxxMethodDecl(hasName("reset"), hasOnlyDefaultParameters()); + + 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 SmartptrResetAmbiguousCallCheck::check( +const MatchFinder::MatchResult &Result) { + + if (const auto *SmartptrResetCall = + Result.Nodes.getNodeAs("smartptrResetCall")) { +const auto *Member = cast(SmartptrResetCall->getCallee()); + +diag(SmartptrResetCall->getBeginLoc(), + "be explicit when calling 'reset()' on a smart pointer with a " + "pointee that has a 'reset()' method"); vbvictor wrote: Done https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::serializeStringList(SmartPointers)); +} + +void SmartptrResetAmbiguousCallCheck::registerMatchers(MatchFinder *Finder) { + const auto IsSmartptr = hasAnyName(SmartPointers); + + const auto ResetMethod = + cxxMethodDecl(hasName("reset"), hasOnlyDefaultParameters()); + + 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( vbvictor wrote: I Could not find a good way to use "anyOf" in this context. If I write like this, it gives CE. ```cpp Finder->addMatcher(anyOf(...)); ``` If I write: ```cpp 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. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor updated https://github.com/llvm/llvm-project/pull/121291 >From 42e03bb9cc9bd815476b0a3f06ac5f58826e3708 Mon Sep 17 00:00:00 2001 From: Victor Baranov Date: Fri, 31 Jan 2025 19:29:05 +0300 Subject: [PATCH 1/3] [clang-tidy] add new check bugprone-reset-ambiguous-call --- .../bugprone/BugproneTidyModule.cpp | 3 + .../clang-tidy/bugprone/CMakeLists.txt| 1 + .../SmartptrResetAmbiguousCallCheck.cpp | 146 +++ .../SmartptrResetAmbiguousCallCheck.h | 37 +++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../smartptr-reset-ambiguous-call.rst | 47 .../docs/clang-tidy/checks/list.rst | 1 + ...r-reset-ambiguous-call-custom-pointers.cpp | 72 ++ .../smartptr-reset-ambiguous-call.cpp | 239 ++ 9 files changed, 552 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/smartptr-reset-ambiguous-call.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/smartptr-reset-ambiguous-call-custom-pointers.cpp create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/smartptr-reset-ambiguous-call.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index c5f0b5b28418f..a01d0e384ab73 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -64,6 +64,7 @@ #include "SignedCharMisuseCheck.h" #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" +#include "SmartptrResetAmbiguousCallCheck.h" #include "SpuriouslyWakeUpFunctionsCheck.h" #include "StandaloneEmptyCheck.h" #include "StringConstructorCheck.h" @@ -207,6 +208,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-sizeof-container"); CheckFactories.registerCheck( "bugprone-sizeof-expression"); +CheckFactories.registerCheck( +"bugprone-smartptr-reset-ambiguous-call"); CheckFactories.registerCheck( "bugprone-spuriously-wake-up-functions"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index e8309c68b7fca..7fd6f4994f537 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -65,6 +65,7 @@ add_clang_library(clangTidyBugproneModule STATIC SizeofContainerCheck.cpp SizeofExpressionCheck.cpp SmartPtrArrayMismatchCheck.cpp + SmartptrResetAmbiguousCallCheck.cpp SpuriouslyWakeUpFunctionsCheck.cpp StandaloneEmptyCheck.cpp StringConstructorCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp new file mode 100644 index 0..326a5665179d7 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp @@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::seriali
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::serializeStringList(SmartPointers)); +} + +void SmartptrResetAmbiguousCallCheck::registerMatchers(MatchFinder *Finder) { + const auto IsSmartptr = hasAnyName(SmartPointers); + + const auto ResetMethod = + cxxMethodDecl(hasName("reset"), hasOnlyDefaultParameters()); + + 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 SmartptrResetAmbiguousCallCheck::check( +const MatchFinder::MatchResult &Result) { + + if (const auto *SmartptrResetCall = + Result.Nodes.getNodeAs("smartptrResetCall")) { +const auto *Member = cast(SmartptrResetCall->getCallee()); + +diag(SmartptrResetCall->getBeginLoc(), + "be explicit when calling 'reset()' on a smart pointer with a " + "pointee that has a 'reset()' method"); PiotrZSL wrote: consider format "whats wrong, how to fix". In this case warning probably could start with `ambiguous call to `reset()` on a smart pointer with pointee that also has a 'reset()' method, prefer more explicit approach` https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::serializeStringList(SmartPointers)); +} + +void SmartptrResetAmbiguousCallCheck::registerMatchers(MatchFinder *Finder) { + const auto IsSmartptr = hasAnyName(SmartPointers); + + const auto ResetMethod = + cxxMethodDecl(hasName("reset"), hasOnlyDefaultParameters()); + + 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( PiotrZSL wrote: you could merge both addMatcher by using "anyOf". https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/PiotrZSL edited https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::serializeStringList(SmartPointers)); +} + +void SmartptrResetAmbiguousCallCheck::registerMatchers(MatchFinder *Finder) { + const auto IsSmartptr = hasAnyName(SmartPointers); + + const auto ResetMethod = + cxxMethodDecl(hasName("reset"), hasOnlyDefaultParameters()); + + 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 SmartptrResetAmbiguousCallCheck::check( +const MatchFinder::MatchResult &Result) { + + if (const auto *SmartptrResetCall = + Result.Nodes.getNodeAs("smartptrResetCall")) { +const auto *Member = cast(SmartptrResetCall->getCallee()); + +diag(SmartptrResetCall->getBeginLoc(), + "be explicit when calling 'reset()' on a smart pointer with a " + "pointee that has a 'reset()' method"); + +diag(SmartptrResetCall->getBeginLoc(), "assign the pointer to 'nullptr'", + DiagnosticIDs::Note) +<< FixItHint::CreateReplacement( + SourceRange(Member->getOperatorLoc(), + Member->getOperatorLoc().getLocWithOffset(0)), + " =") +<< FixItHint::CreateReplacement( + SourceRange(Member->getMemberLoc(), + SmartptrResetCall->getEndLoc()), + " nullptr"); +return; + } + + if (const auto *ObjectResetCall = + Result.Nodes.getNodeAs("objectResetCall")) { +const auto *Arrow = Result.Nodes.getNodeAs("OpCall"); + +const CharSourceRange SmartptrSourceRange = +Lexer::getAsCharRange(Arrow->getArg(0)->getSourceRange(), + *Result.SourceManager, getLangOpts()); + +diag(ObjectResetCall->getBeginLoc(), + "be explicit when calling 'reset()' on a pointee of a smart pointer"); PiotrZSL wrote: same comments related to msg as before https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@list
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::serializeStringList(SmartPointers)); +} + +void SmartptrResetAmbiguousCallCheck::registerMatchers(MatchFinder *Finder) { + const auto IsSmartptr = hasAnyName(SmartPointers); + + const auto ResetMethod = + cxxMethodDecl(hasName("reset"), hasOnlyDefaultParameters()); + + 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 SmartptrResetAmbiguousCallCheck::check( +const MatchFinder::MatchResult &Result) { + + if (const auto *SmartptrResetCall = + Result.Nodes.getNodeAs("smartptrResetCall")) { +const auto *Member = cast(SmartptrResetCall->getCallee()); + +diag(SmartptrResetCall->getBeginLoc(), + "be explicit when calling 'reset()' on a smart pointer with a " + "pointee that has a 'reset()' method"); + +diag(SmartptrResetCall->getBeginLoc(), "assign the pointer to 'nullptr'", PiotrZSL wrote: again, `consider assigning 'nullptr' here` https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,34 @@ +//===--- SmartptrResetAmbiguousCallCheck.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_SMARTPTRRESETAMBIGUOUSCALLCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SMARTPTRRESETAMBIGUOUSCALLCHECK_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/smartptr-reset-ambiguous-call.html +class SmartptrResetAmbiguousCallCheck : public ClangTidyCheck { +public: + SmartptrResetAmbiguousCallCheck(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; PiotrZSL wrote: Ok, but then add "boost::shared_ptr" to default configuration https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,47 @@ +.. title:: clang-tidy - bugprone-smartptr-reset-ambiguous-call + +bugprone-smartptr-reset-ambiguous-call +== + +Finds potentially erroneous calls to ``reset`` method on smart pointers when +the pointee type also has a ``reset`` method. Having a ``reset`` method in +both classes 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(); + + 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 a 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 ptr = std::make_unique(); + + (*ptr).reset(); // Clearly calls underlying reset method + ptr = nullptr; // Clearly makes the pointer null + +The default smart pointers that are considered are ``std::unique_ptr``, +``std::shared_ptr``. To specify other smart pointers or other classes use the +:option:`SmartPointers` option. + +Options +--- + +.. option:: SmartPointers + +Semicolon-separated list of class names of custom smart pointers. PiotrZSL wrote: add fully qualified https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/PiotrZSL commented: 1. Overall ok, please work a little bit on an warning message. 2. Probably better name would be: readability-ambiguous-smartptr-reset-call 3. Same issue exist on other objects like std::optional, and other methods like std::optional 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. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
vbvictor wrote: @PiotrZSL, @5chmidti Ping https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
vbvictor wrote: Ping https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor updated https://github.com/llvm/llvm-project/pull/121291 >From 42e03bb9cc9bd815476b0a3f06ac5f58826e3708 Mon Sep 17 00:00:00 2001 From: Victor Baranov Date: Fri, 31 Jan 2025 19:29:05 +0300 Subject: [PATCH 1/2] [clang-tidy] add new check bugprone-reset-ambiguous-call --- .../bugprone/BugproneTidyModule.cpp | 3 + .../clang-tidy/bugprone/CMakeLists.txt| 1 + .../SmartptrResetAmbiguousCallCheck.cpp | 146 +++ .../SmartptrResetAmbiguousCallCheck.h | 37 +++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../smartptr-reset-ambiguous-call.rst | 47 .../docs/clang-tidy/checks/list.rst | 1 + ...r-reset-ambiguous-call-custom-pointers.cpp | 72 ++ .../smartptr-reset-ambiguous-call.cpp | 239 ++ 9 files changed, 552 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/smartptr-reset-ambiguous-call.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/smartptr-reset-ambiguous-call-custom-pointers.cpp create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/smartptr-reset-ambiguous-call.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index c5f0b5b28418f8..a01d0e384ab737 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -64,6 +64,7 @@ #include "SignedCharMisuseCheck.h" #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" +#include "SmartptrResetAmbiguousCallCheck.h" #include "SpuriouslyWakeUpFunctionsCheck.h" #include "StandaloneEmptyCheck.h" #include "StringConstructorCheck.h" @@ -207,6 +208,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-sizeof-container"); CheckFactories.registerCheck( "bugprone-sizeof-expression"); +CheckFactories.registerCheck( +"bugprone-smartptr-reset-ambiguous-call"); CheckFactories.registerCheck( "bugprone-spuriously-wake-up-functions"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index e8309c68b7fcaa..7fd6f4994f5375 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -65,6 +65,7 @@ add_clang_library(clangTidyBugproneModule STATIC SizeofContainerCheck.cpp SizeofExpressionCheck.cpp SmartPtrArrayMismatchCheck.cpp + SmartptrResetAmbiguousCallCheck.cpp SpuriouslyWakeUpFunctionsCheck.cpp StandaloneEmptyCheck.cpp StringConstructorCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp new file mode 100644 index 00..326a5665179d79 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp @@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::s
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor updated https://github.com/llvm/llvm-project/pull/121291 >From 42e03bb9cc9bd815476b0a3f06ac5f58826e3708 Mon Sep 17 00:00:00 2001 From: Victor Baranov Date: Fri, 31 Jan 2025 19:29:05 +0300 Subject: [PATCH] [clang-tidy] add new check bugprone-reset-ambiguous-call --- .../bugprone/BugproneTidyModule.cpp | 3 + .../clang-tidy/bugprone/CMakeLists.txt| 1 + .../SmartptrResetAmbiguousCallCheck.cpp | 146 +++ .../SmartptrResetAmbiguousCallCheck.h | 37 +++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../smartptr-reset-ambiguous-call.rst | 47 .../docs/clang-tidy/checks/list.rst | 1 + ...r-reset-ambiguous-call-custom-pointers.cpp | 72 ++ .../smartptr-reset-ambiguous-call.cpp | 239 ++ 9 files changed, 552 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/smartptr-reset-ambiguous-call.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/smartptr-reset-ambiguous-call-custom-pointers.cpp create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/smartptr-reset-ambiguous-call.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp index c5f0b5b28418f8..a01d0e384ab737 100644 --- a/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp @@ -64,6 +64,7 @@ #include "SignedCharMisuseCheck.h" #include "SizeofContainerCheck.h" #include "SizeofExpressionCheck.h" +#include "SmartptrResetAmbiguousCallCheck.h" #include "SpuriouslyWakeUpFunctionsCheck.h" #include "StandaloneEmptyCheck.h" #include "StringConstructorCheck.h" @@ -207,6 +208,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-sizeof-container"); CheckFactories.registerCheck( "bugprone-sizeof-expression"); +CheckFactories.registerCheck( +"bugprone-smartptr-reset-ambiguous-call"); CheckFactories.registerCheck( "bugprone-spuriously-wake-up-functions"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt index e8309c68b7fcaa..7fd6f4994f5375 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt @@ -65,6 +65,7 @@ add_clang_library(clangTidyBugproneModule STATIC SizeofContainerCheck.cpp SizeofExpressionCheck.cpp SmartPtrArrayMismatchCheck.cpp + SmartptrResetAmbiguousCallCheck.cpp SpuriouslyWakeUpFunctionsCheck.cpp StandaloneEmptyCheck.cpp StringConstructorCheck.cpp diff --git a/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp new file mode 100644 index 00..326a5665179d79 --- /dev/null +++ b/clang-tools-extra/clang-tidy/bugprone/SmartptrResetAmbiguousCallCheck.cpp @@ -0,0 +1,146 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + for (const auto *Arg : Node.arguments()) { +if (!InnerMatcher.matches(*Arg, Finder, Builder)) + return false; + } + + return true; +} + +AST_MATCHER(CXXMethodDecl, hasOnlyDefaultParameters) { + for (const auto *Param : Node.parameters()) { +if (!Param->hasDefaultArg()) + return false; + } + + return true; +} + +const auto DefaultSmartPointers = "::std::shared_ptr;::std::unique_ptr"; +} // namespace + +SmartptrResetAmbiguousCallCheck::SmartptrResetAmbiguousCallCheck( +StringRef Name, ClangTidyContext *Context) +: ClangTidyCheck(Name, Context), + SmartPointers(utils::options::parseStringList( + Options.get("SmartPointers", DefaultSmartPointers))) {} + +void SmartptrResetAmbiguousCallCheck::storeOptions( +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "SmartPointers", +utils::options::seria
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
EugeneZelenko wrote: Please rebase from `main`. Release Notes were cleared after 20 branch creation. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
vbvictor wrote: Ping https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,48 @@ +.. title:: clang-tidy - bugprone-smartptr-reset-ambiguous-call + +bugprone-smartptr-reset-ambiguous-call +== + +Finds potentially erroneous calls to ``reset`` method on vbvictor wrote: Done https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor updated https://github.com/llvm/llvm-project/pull/121291 >From 37dce6a7ed0cca2e9819c24f4d176c43e3c9f2ac Mon Sep 17 00:00:00 2001 From: Victor Baranov Date: Sun, 29 Dec 2024 15:32:22 +0300 Subject: [PATCH 1/8] [clang-tidy] Add bugprone-reset-call check --- .../bugprone/BugproneTidyModule.cpp | 2 + .../clang-tidy/bugprone/CMakeLists.txt| 1 + .../clang-tidy/bugprone/ResetCallCheck.cpp| 133 +++ .../clang-tidy/bugprone/ResetCallCheck.h | 34 +++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../clang-tidy/checks/bugprone/reset-call.rst | 33 +++ .../docs/clang-tidy/checks/list.rst | 1 + .../checkers/bugprone/reset-call.cpp | 215 ++ 8 files changed, 425 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/reset-call.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/reset-call.cpp 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( "bugprone-incorrect-enable-if"); +CheckFactories.registerCheck("bugprone-reset-call"); CheckFactories.registerCheck( "bugprone-return-const-ref-from-parameter"); CheckFactories.registerCheck( 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 00..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, 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", +
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,48 @@ +.. title:: clang-tidy - bugprone-smartptr-reset-ambiguous-call + +bugprone-smartptr-reset-ambiguous-call +== + +Finds potentially erroneous calls to ``reset`` method on EugeneZelenko wrote: Please fill as much as possible to 80-characters limit. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/HerrCai0907 approved this pull request. LGTM. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,150 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, 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, hasOnlyDefaultParameters) { + if (Node.param_empty()) vbvictor wrote: Yes, removed useless condition. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,150 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + if (Node.getNumArgs() == 0) +return true; + vbvictor wrote: Yes, removed useless condition. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor updated https://github.com/llvm/llvm-project/pull/121291 >From 37dce6a7ed0cca2e9819c24f4d176c43e3c9f2ac Mon Sep 17 00:00:00 2001 From: Victor Baranov Date: Sun, 29 Dec 2024 15:32:22 +0300 Subject: [PATCH 1/7] [clang-tidy] Add bugprone-reset-call check --- .../bugprone/BugproneTidyModule.cpp | 2 + .../clang-tidy/bugprone/CMakeLists.txt| 1 + .../clang-tidy/bugprone/ResetCallCheck.cpp| 133 +++ .../clang-tidy/bugprone/ResetCallCheck.h | 34 +++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../clang-tidy/checks/bugprone/reset-call.rst | 33 +++ .../docs/clang-tidy/checks/list.rst | 1 + .../checkers/bugprone/reset-call.cpp | 215 ++ 8 files changed, 425 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/reset-call.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/reset-call.cpp 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( "bugprone-incorrect-enable-if"); +CheckFactories.registerCheck("bugprone-reset-call"); CheckFactories.registerCheck( "bugprone-return-const-ref-from-parameter"); CheckFactories.registerCheck( 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 00..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, 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", +
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,150 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, InnerMatcher) { + if (Node.getNumArgs() == 0) +return true; + HerrCai0907 wrote: looks like useless since it is return at the end if it is empty. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,150 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.h" +#include "../utils/OptionsUtils.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, 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, hasOnlyDefaultParameters) { + if (Node.param_empty()) HerrCai0907 wrote: same https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
vbvictor wrote: Thank you everyone for the feedback! @PiotrZSL I tried using TK_IgnoreUnlessSpelledInSource with `cxxDependentScopeMemberExpr` ast-matcher but failed to write a working matcher case with nested template parameters: ```cpp template void TemplatePositiveTest() { std::unique_ptr u_ptr; u_ptr.reset(); u_ptr->reset(); } void instantiate() { TemplatePositiveTest>(); } ``` 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. Apart from TK_IgnoreUnlessSpelledInSource I fixed your comments. @5chmidti I fixed all your comments and added better _warning_ and _note_ messages: ```cpp 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. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,47 @@ +.. title:: clang-tidy - bugprone-smartptr-reset-ambiguous-call + +bugprone-smartptr-reset-ambiguous-call +== + +Finds potentially erroneous calls to ``reset`` method on +smart pointers when the pointee type also has a ``reset`` method. +Having a ``reset`` method in both classes 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(); + + 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 a 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 ptr = std::make_unique(); + + (*ptr).reset(); // Clearly calls underlying reset method + ptr = nullptr; // Clearly makes the pointer null + +The default smart pointers that are considered are ``std::unique_ptr``, +``std::shared_ptr``. To specify other smart pointers or +other classes use the :option:`SmartPointers` option. + +Options +--- + +.. option:: SmartPointers + +Semicolon-separated list of class names of custom smart pointers. vbvictor wrote: Done https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor updated https://github.com/llvm/llvm-project/pull/121291 >From 37dce6a7ed0cca2e9819c24f4d176c43e3c9f2ac Mon Sep 17 00:00:00 2001 From: Victor Baranov Date: Sun, 29 Dec 2024 15:32:22 +0300 Subject: [PATCH 1/6] [clang-tidy] Add bugprone-reset-call check --- .../bugprone/BugproneTidyModule.cpp | 2 + .../clang-tidy/bugprone/CMakeLists.txt| 1 + .../clang-tidy/bugprone/ResetCallCheck.cpp| 133 +++ .../clang-tidy/bugprone/ResetCallCheck.h | 34 +++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../clang-tidy/checks/bugprone/reset-call.rst | 33 +++ .../docs/clang-tidy/checks/list.rst | 1 + .../checkers/bugprone/reset-call.cpp | 215 ++ 8 files changed, 425 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/reset-call.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/reset-call.cpp 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( "bugprone-incorrect-enable-if"); +CheckFactories.registerCheck("bugprone-reset-call"); CheckFactories.registerCheck( "bugprone-return-const-ref-from-parameter"); CheckFactories.registerCheck( 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 00..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, 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", +
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,47 @@ +.. title:: clang-tidy - bugprone-smartptr-reset-ambiguous-call + +bugprone-smartptr-reset-ambiguous-call +== + +Finds potentially erroneous calls to ``reset`` method on +smart pointers when the pointee type also has a ``reset`` method. +Having a ``reset`` method in both classes 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(); + + 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 a 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 ptr = std::make_unique(); + + (*ptr).reset(); // Clearly calls underlying reset method + ptr = nullptr; // Clearly makes the pointer null + +The default smart pointers that are considered are ``std::unique_ptr``, +``std::shared_ptr``. To specify other smart pointers or +other classes use the :option:`SmartPointers` option. + +Options +--- + +.. option:: SmartPointers + +Semicolon-separated list of class names of custom smart pointers. EugeneZelenko wrote: Please add default value. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor updated https://github.com/llvm/llvm-project/pull/121291 >From 37dce6a7ed0cca2e9819c24f4d176c43e3c9f2ac Mon Sep 17 00:00:00 2001 From: Victor Baranov Date: Sun, 29 Dec 2024 15:32:22 +0300 Subject: [PATCH 1/5] [clang-tidy] Add bugprone-reset-call check --- .../bugprone/BugproneTidyModule.cpp | 2 + .../clang-tidy/bugprone/CMakeLists.txt| 1 + .../clang-tidy/bugprone/ResetCallCheck.cpp| 133 +++ .../clang-tidy/bugprone/ResetCallCheck.h | 34 +++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../clang-tidy/checks/bugprone/reset-call.rst | 33 +++ .../docs/clang-tidy/checks/list.rst | 1 + .../checkers/bugprone/reset-call.cpp | 215 ++ 8 files changed, 425 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/reset-call.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/reset-call.cpp 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( "bugprone-incorrect-enable-if"); +CheckFactories.registerCheck("bugprone-reset-call"); CheckFactories.registerCheck( "bugprone-return-const-ref-from-parameter"); CheckFactories.registerCheck( 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 00..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, 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", +
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,34 @@ +.. title:: clang-tidy - bugprone-smartptr-reset-ambiguous-call + +bugprone-smartptr-reset-ambiguous-call +== + +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 +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(); + + 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. vbvictor wrote: Done https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -144,6 +145,8 @@ class BugproneModule : public ClangTidyModule { "bugprone-inaccurate-erase"); CheckFactories.registerCheck( "bugprone-incorrect-enable-if"); +CheckFactories.registerCheck( +"bugprone-smartptr-reset-ambiguous-call"); vbvictor wrote: Done https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,134 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.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, 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 SmartptrResetAmbiguousCallCheck::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 SmartptrResetAmbiguousCallCheck::check( +const MatchFinder::MatchResult &Result) { + const auto *SmartptrResetCall = + Result.Nodes.getNodeAs("smartptrResetCall"); + const auto *ObjectResetCall = + Result.Nodes.getNodeAs("objectResetCall"); + + if (SmartptrResetCall) { +const auto *Member = cast(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("OpCall"); + +const auto SmartptrSourceRange = vbvictor wrote: Done https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,134 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.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, 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 SmartptrResetAmbiguousCallCheck::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 SmartptrResetAmbiguousCallCheck::check( +const MatchFinder::MatchResult &Result) { + const auto *SmartptrResetCall = + Result.Nodes.getNodeAs("smartptrResetCall"); + const auto *ObjectResetCall = + Result.Nodes.getNodeAs("objectResetCall"); vbvictor wrote: Done https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,34 @@ +.. title:: clang-tidy - bugprone-smartptr-reset-ambiguous-call + +bugprone-smartptr-reset-ambiguous-call +== + +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 vbvictor wrote: Done https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,34 @@ +.. title:: clang-tidy - bugprone-smartptr-reset-ambiguous-call + +bugprone-smartptr-reset-ambiguous-call +== + +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 +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(); + + 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: vbvictor wrote: Done https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,34 @@ +//===--- SmartptrResetAmbiguousCallCheck.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_SMARTPTRRESETAMBIGUOUSCALLCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SMARTPTRRESETAMBIGUOUSCALLCHECK_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/smartptr-reset-ambiguous-call.html +class SmartptrResetAmbiguousCallCheck : public ClangTidyCheck { +public: + SmartptrResetAmbiguousCallCheck(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; vbvictor wrote: I added an option to specify custom smart pointers. Now `boost::shared_ptr` can be valid pointer for this check, so I did not change to `CPlusPlus11`. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,134 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.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, 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 SmartptrResetAmbiguousCallCheck::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 SmartptrResetAmbiguousCallCheck::check( +const MatchFinder::MatchResult &Result) { + const auto *SmartptrResetCall = + Result.Nodes.getNodeAs("smartptrResetCall"); + const auto *ObjectResetCall = + Result.Nodes.getNodeAs("objectResetCall"); + + if (SmartptrResetCall) { +const auto *Member = cast(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"); + } vbvictor wrote: Done https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor updated https://github.com/llvm/llvm-project/pull/121291 >From 37dce6a7ed0cca2e9819c24f4d176c43e3c9f2ac Mon Sep 17 00:00:00 2001 From: Victor Baranov Date: Sun, 29 Dec 2024 15:32:22 +0300 Subject: [PATCH 1/4] [clang-tidy] Add bugprone-reset-call check --- .../bugprone/BugproneTidyModule.cpp | 2 + .../clang-tidy/bugprone/CMakeLists.txt| 1 + .../clang-tidy/bugprone/ResetCallCheck.cpp| 133 +++ .../clang-tidy/bugprone/ResetCallCheck.h | 34 +++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../clang-tidy/checks/bugprone/reset-call.rst | 33 +++ .../docs/clang-tidy/checks/list.rst | 1 + .../checkers/bugprone/reset-call.cpp | 215 ++ 8 files changed, 425 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/bugprone/ResetCallCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/bugprone/reset-call.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/bugprone/reset-call.cpp 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( "bugprone-incorrect-enable-if"); +CheckFactories.registerCheck("bugprone-reset-call"); CheckFactories.registerCheck( "bugprone-return-const-ref-from-parameter"); CheckFactories.registerCheck( 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 00..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, 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", +
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor reopened https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
vbvictor wrote: > 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 > What do others think? https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor closed https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor edited https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
https://github.com/vbvictor edited https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,134 @@ +//===--- SmartptrResetAmbiguousCallCheck.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 "SmartptrResetAmbiguousCallCheck.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, 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 SmartptrResetAmbiguousCallCheck::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 SmartptrResetAmbiguousCallCheck::check( +const MatchFinder::MatchResult &Result) { + const auto *SmartptrResetCall = + Result.Nodes.getNodeAs("smartptrResetCall"); + const auto *ObjectResetCall = + Result.Nodes.getNodeAs("objectResetCall"); + + if (SmartptrResetCall) { +const auto *Member = cast(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("OpCall"); + +const auto SmartptrSourceRange = EugeneZelenko wrote: `auto` should not be used here - type is not spelled explicitly. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,34 @@ +//===--- SmartptrResetAmbiguousCallCheck.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_SMARTPTRRESETAMBIGUOUSCALLCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SMARTPTRRESETAMBIGUOUSCALLCHECK_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/smartptr-reset-ambiguous-call.html +class SmartptrResetAmbiguousCallCheck : public ClangTidyCheck { +public: + SmartptrResetAmbiguousCallCheck(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; vbvictor wrote: As for now, this check only matches `::std::unique_ptr` and `::std::shared_ptr`, so this comment is valid. In the future, I think a list of smartptr-like classes should be an option for check. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Add bugprone-smartptr-reset-ambiguous-call check (PR #121291)
@@ -0,0 +1,34 @@ +//===--- SmartptrResetAmbiguousCallCheck.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_SMARTPTRRESETAMBIGUOUSCALLCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_SMARTPTRRESETAMBIGUOUSCALLCHECK_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/smartptr-reset-ambiguous-call.html +class SmartptrResetAmbiguousCallCheck : public ClangTidyCheck { +public: + SmartptrResetAmbiguousCallCheck(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; denzor200 wrote: I disagree, the code issue this check is hitting for also relevant for C++03. Think about `boost::shared_ptr`, for example. https://github.com/llvm/llvm-project/pull/121291 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits