[clang-tools-extra] [clang-tidy] Add bugprone-move-shared-pointer-contents check. (PR #67467)
denzor200 wrote: > Wouldn't that be a runtime check, and thus not something clang-tidy can do > without significant data flow analysis effort? I'm not experienced enough in clang-tidy internals, but I believe it will not be a serious challenge to check for having `assert(sp.use_count() == 1)` one line before `*sp` being moved. I suppose everybody agree that this snippet ``` assert(sp.use_count() == 1); auto y = std::move(*sp); ``` ..is better than that one: ``` // NOLINTNEXTLINE(clang-tidy-bugprone-move-shared-pointer-contents) auto y = std::move(*sp); ``` https://github.com/llvm/llvm-project/pull/67467 ___ 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-move-shared-pointer-contents check. (PR #67467)
denzor200 wrote: Suppose this check should be silent for a shared_ptr with `use_count() == 1`. Is it possible to change implementation in order to following this way? https://github.com/llvm/llvm-project/pull/67467 ___ 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
[clang-tools-extra] [clang-tidy] Added bugprone-exception-rethrow check (PR #86448)
@@ -0,0 +1,103 @@ +// RUN: %check_clang_tidy %s bugprone-exception-rethrow %t -- -- -fexceptions + +struct exception {}; + +namespace std { + template + T&& move(T &&x) { +return static_cast(x); + } +} + +void correct() { + try { + throw exception(); + } catch(const exception &) { + throw; + } +} + +void correct2() { + try { + throw exception(); + } catch(const exception &e) { + try { +throw exception(); + } catch(...) {} + } +} + +void not_correct() { + try { + throw exception(); + } catch(const exception &e) { + throw e; +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: throwing a copy of the caught 'exception' exception, remove the argument to throw the original exception object [bugprone-exception-rethrow] + } +} + +void not_correct2() { + try { + throw exception(); + } catch(const exception &e) { + throw (e); +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: throwing a copy of the caught 'exception' exception, remove the argument to throw the original exception object [bugprone-exception-rethrow] + } +} + +void not_correct3() { + try { + throw exception(); + } catch(const exception &e) { + throw exception(e); +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: throwing a copy of the caught 'exception' exception, remove the argument to throw the original exception object [bugprone-exception-rethrow] + } +} + +void not_correct4() { + try { + throw exception(); + } catch(exception &e) { + throw std::move(e); +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: throwing a copy of the caught 'exception' exception, remove the argument to throw the original exception object [bugprone-exception-rethrow] + } +} + +void not_correct5() { + try { + throw 5; + } catch(const int &e) { + throw e; +// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: throwing a copy of the caught 'int' exception, remove the argument to throw the original exception object [bugprone-exception-rethrow] + } +} + denzor200 wrote: ``` void not_correct6() { try { throw exception(); } catch(const exception &e) { exception e1 = e; throw e1; // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: throwing a copy of the caught 'exception' exception, remove the argument to throw the original exception object [bugprone-exception-rethrow] } } ``` please https://github.com/llvm/llvm-project/pull/86448 ___ 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 readability-string-view-substr check (PR #120055)
@@ -0,0 +1,108 @@ +// RUN: %check_clang_tidy %s readability-stringview-substr %t + +namespace std { +template +class basic_string_view { +public: + using size_type = unsigned long; + static constexpr size_type npos = -1; + + basic_string_view(const char*) {} + basic_string_view substr(size_type pos, size_type count = npos) const { return *this; } + void remove_prefix(size_type n) {} + void remove_suffix(size_type n) {} + size_type length() const { return 0; } + basic_string_view& operator=(const basic_string_view&) { return *this; } +}; + +using string_view = basic_string_view; +} // namespace std + +void test_basic() { + std::string_view sv("test"); + std::string_view sv1("test"); + std::string_view sv2("test"); + + // Should match: remove_prefix + sv = sv.substr(5); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_prefix' over 'substr' for removing characters from the start [readability-stringview-substr] + // CHECK-FIXES: sv.remove_prefix(5) + + // Should match: remove_suffix + sv = sv.substr(0, sv.length() - 3); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr] + // CHECK-FIXES: sv.remove_suffix(3) + + // Should match: remove_suffix with complex expression + sv = sv.substr(0, sv.length() - (3 + 2)); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr] + // CHECK-FIXES: sv.remove_suffix((3 + 2)) +} + +void test_copies() { + std::string_view sv("test"); + std::string_view sv1("test"); + std::string_view sv2("test"); + + // Should match: remove redundant self copies + sv = sv.substr(0, sv.length()); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant self-copy [readability-stringview-substr] + + sv = sv.substr(0, sv.length() - 0); +// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: redundant self-copy [readability-stringview-substr] + + // Should match: simplify copies between different variables + sv1 = sv.substr(0, sv.length()); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer direct copy over substr [readability-stringview-substr] + // CHECK-FIXES: sv1 = sv + + sv2 = sv.substr(0, sv.length() - 0); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer direct copy over substr [readability-stringview-substr] + // CHECK-FIXES: sv2 = sv +} + +void test_zero_forms() { + std::string_view sv("test"); + const int kZero = 0; + constexpr std::string_view::size_type Zero = 0; + #define START_POS 0 + + // Should match: various forms of zero in first argument + sv = sv.substr(kZero, sv.length() - 3); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr] + // CHECK-FIXES: sv.remove_suffix(3) + + sv = sv.substr(Zero, sv.length() - 3); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr] + // CHECK-FIXES: sv.remove_suffix(3) + + sv = sv.substr(START_POS, sv.length() - 3); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr] + // CHECK-FIXES: sv.remove_suffix(3) + + sv = sv.substr((0), sv.length() - 3); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr] + // CHECK-FIXES: sv.remove_suffix(3) + + sv = sv.substr(0u, sv.length() - 3); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr] + // CHECK-FIXES: sv.remove_suffix(3) + + sv = sv.substr(0UL, sv.length() - 3); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer 'remove_suffix' over 'substr' for removing characters from the end [readability-stringview-substr] + // CHECK-FIXES: sv.remove_suffix(3) +} + +void test_should_not_match() { + std::string_view sv("test"); + std::string_view sv1("test"); + std::string_view sv2("test"); + + // No match: substr used for prefix or mid-view + sv = sv.substr(1, sv.length() - 3); // No warning + + // No match: Different string_views + sv = sv2.substr(0, sv2.length() - 3); // No warning denzor200 wrote: Why no match? I don't see any trouble with ``` sv = sv2; sv.remove_suffix(3); ``` Of couse still no match for const version of `sv` https://github.com/llvm/llvm-project/pull/120055 ___ 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 readability-string-view-substr check (PR #120055)
@@ -0,0 +1,108 @@ +// RUN: %check_clang_tidy %s readability-stringview-substr %t + +namespace std { +template +class basic_string_view { +public: + using size_type = unsigned long; + static constexpr size_type npos = -1; + + basic_string_view(const char*) {} + basic_string_view substr(size_type pos, size_type count = npos) const { return *this; } + void remove_prefix(size_type n) {} + void remove_suffix(size_type n) {} + size_type length() const { return 0; } denzor200 wrote: Please add `size_type size() const { return 0; }` and unit-tests for it https://github.com/llvm/llvm-project/pull/120055 ___ 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 readability-use-span-first-last check (PR #118074)
@@ -0,0 +1,149 @@ +// RUN: %check_clang_tidy -std=c++20 %s readability-use-span-first-last %t + +namespace std { +template +class span { + T* ptr; + __SIZE_TYPE__ len; + +public: + span(T* p, __SIZE_TYPE__ l) : ptr(p), len(l) {} + + span subspan(__SIZE_TYPE__ offset) const { +return span(ptr + offset, len - offset); + } + + span subspan(__SIZE_TYPE__ offset, __SIZE_TYPE__ count) const { +return span(ptr + offset, count); + } + + span first(__SIZE_TYPE__ count) const { +return span(ptr, count); + } + + span last(__SIZE_TYPE__ count) const { +return span(ptr + (len - count), count); + } + + __SIZE_TYPE__ size() const { return len; } +}; +} // namespace std + +// Add here, right after the std namespace closes: +namespace std::ranges { + template + __SIZE_TYPE__ size(const span& s) { return s.size(); } +} + +void test() { + int arr[] = {1, 2, 3, 4, 5}; + std::span s(arr, 5); + + auto sub1 = s.subspan(0, 3); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer 'span::first()' over 'subspan()' + // CHECK-FIXES: auto sub1 = s.first(3); + + auto sub2 = s.subspan(s.size() - 2); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer 'span::last()' over 'subspan()' + // CHECK-FIXES: auto sub2 = s.last(2); + + __SIZE_TYPE__ n = 2; + auto sub3 = s.subspan(0, n); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer 'span::first()' over 'subspan()' + // CHECK-FIXES: auto sub3 = s.first(n); + + auto sub4 = s.subspan(1, 2); // No warning + auto sub5 = s.subspan(2); // No warning + + +#define ZERO 0 +#define TWO 2 +#define SIZE_MINUS(s, n) s.size() - n +#define MAKE_SUBSPAN(obj, n) obj.subspan(0, n) +#define MAKE_LAST_N(obj, n) obj.subspan(obj.size() - n) + + auto sub6 = s.subspan(SIZE_MINUS(s, 2)); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer 'span::last()' over 'subspan()' + // CHECK-FIXES: auto sub6 = s.last(2); + + auto sub7 = MAKE_SUBSPAN(s, 3); + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: prefer 'span::first()' over 'subspan()' + // CHECK-FIXES: auto sub7 = s.first(3); + + auto sub8 = MAKE_LAST_N(s, 2); + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: prefer 'span::last()' over 'subspan()' + // CHECK-FIXES: auto sub8 = s.last(2); + +} + +template +void testTemplate() { + T arr[] = {1, 2, 3, 4, 5}; + std::span s(arr, 5); + + auto sub1 = s.subspan(0, 3); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer 'span::first()' over 'subspan()' + // CHECK-FIXES: auto sub1 = s.first(3); + + auto sub2 = s.subspan(s.size() - 2); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer 'span::last()' over 'subspan()' + // CHECK-FIXES: auto sub2 = s.last(2); + + __SIZE_TYPE__ n = 2; + auto sub3 = s.subspan(0, n); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer 'span::first()' over 'subspan()' + // CHECK-FIXES: auto sub3 = s.first(n); + + auto sub4 = s.subspan(1, 2); // No warning + auto sub5 = s.subspan(2); // No warning + + auto complex = s.subspan(0 + (s.size() - 2), 3); // No warning + + auto complex2 = s.subspan(100 + (s.size() - 2)); // No warning +} + +// Test instantiation +void testInt() { + testTemplate(); +} + +void test_ranges() { + int arr[] = {1, 2, 3, 4, 5}; + std::span s(arr, 5); + + auto sub1 = s.subspan(std::ranges::size(s) - 2); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer 'span::last()' over 'subspan()' + // CHECK-FIXES: auto sub1 = s.last(2); + + __SIZE_TYPE__ n = 2; + auto sub2 = s.subspan(std::ranges::size(s) - n); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer 'span::last()' over 'subspan()' + // CHECK-FIXES: auto sub2 = s.last(n); +} + +void test_different_spans() { + int arr1[] = {1, 2, 3, 4, 5}; + int arr2[] = {6, 7, 8, 9, 10}; + std::span s1(arr1, 5); + std::span s2(arr2, 5); + + // These should NOT trigger warnings as they use size() from a different span + auto sub1 = s1.subspan(s2.size() - 2); // No warning + auto sub2 = s2.subspan(s1.size() - 3); // No warning + + // Also check with std::ranges::size + auto sub3 = s1.subspan(std::ranges::size(s2) - 2); // No warning + auto sub4 = s2.subspan(std::ranges::size(s1) - 3); // No warning + + // Mixed usage should also not trigger + auto sub5 = s1.subspan(s2.size() - s1.size()); // No warning + + // Verify that correct usage still triggers warnings + auto good1 = s1.subspan(s1.size() - 2); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer 'span::last()' over 'subspan()' + // CHECK-FIXES: auto good1 = s1.last(2); + + auto good2 = s2.subspan(std::ranges::size(s2) - 3); + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer 'span::last()' over 'subspan()' + // CHECK-FIXES: auto good2 = s2.last(3); +} denzor200 wrote: please add `test_span_of_bytes`: ``` std::byte arr[] = {0x1, 0x2, 0x3, 0x4, 0x5}; std::span s(arr, 5); // ... auto sub2 = s.subspan(s.size_bytes() - 2); // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: prefer 'span::last()'
[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)
@@ -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)
@@ -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/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,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)
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 new check bugprone-capture-this-by-field (PR #130297)
@@ -0,0 +1,99 @@ +//===--- CaptureThisByFieldCheck.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 "CaptureThisByFieldCheck.h" +#include "clang/AST/DeclCXX.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/ASTMatchers/ASTMatchersMacros.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::bugprone { + +namespace { + +AST_MATCHER(CXXRecordDecl, correctHandleCaptureThisLambda) { + // unresolved + if (Node.needsOverloadResolutionForCopyConstructor() && + Node.needsImplicitCopyConstructor()) +return false; + if (Node.needsOverloadResolutionForMoveConstructor() && + Node.needsImplicitMoveConstructor()) +return false; + if (Node.needsOverloadResolutionForCopyAssignment() && + Node.needsImplicitCopyAssignment()) +return false; + if (Node.needsOverloadResolutionForMoveAssignment() && + Node.needsImplicitMoveAssignment()) +return false; + // default but not deleted + if (Node.hasSimpleCopyConstructor()) +return false; + if (Node.hasSimpleMoveConstructor()) +return false; + if (Node.hasSimpleCopyAssignment()) +return false; + if (Node.hasSimpleMoveAssignment()) +return false; + + for (CXXConstructorDecl const *C : Node.ctors()) { +if (C->isCopyOrMoveConstructor() && C->isDefaulted() && !C->isDeleted()) + return false; + } + for (CXXMethodDecl const *M : Node.methods()) { +if (M->isCopyAssignmentOperator() && M->isDefaulted() && !M->isDeleted()) + return false; +if (M->isMoveAssignmentOperator() && M->isDefaulted() && !M->isDeleted()) + return false; + } + // FIXME: find ways to identifier correct handle capture this lambda + return true; +} + +} // namespace + +void CaptureThisByFieldCheck::registerMatchers(MatchFinder *Finder) { + auto IsStdFunctionField = + fieldDecl(hasType(cxxRecordDecl(hasName("::std::function" denzor200 wrote: Needs an option to add a custom function class, `boost::function` for example https://github.com/llvm/llvm-project/pull/130297 ___ 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 new check bugprone-capture-this-by-field (PR #130297)
@@ -118,6 +119,8 @@ class BugproneModule : public ClangTidyModule { CheckFactories.registerCheck( "bugprone-bool-pointer-implicit-conversion"); CheckFactories.registerCheck("bugprone-branch-clone"); +CheckFactories.registerCheck( +"bugprone-capture-this-by-field"); denzor200 wrote: `bugprone-capturing-this-by-field` ? https://github.com/llvm/llvm-project/pull/130297 ___ 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 new check bugprone-capture-this-by-field (PR #130297)
https://github.com/denzor200 requested changes to this pull request. https://github.com/llvm/llvm-project/pull/130297 ___ 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 new check bugprone-capture-this-by-field (PR #130297)
@@ -0,0 +1,144 @@ +// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-capturing-this-by-field %t -- -config="{CheckOptions: {bugprone-capturing-this-by-field.FunctionWrapperTypes: '::std::function;::Fn'}}" -- + +namespace std { + +template +class function; + +template +class function { +public: + function() noexcept; + template function(F &&); +}; + +} // namespace std + +struct Fn { + template Fn(F &&); +}; + +struct Basic { + Basic() : Captured([this]() { static_cast(this); }) {} + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: using lambda expressions to capture 'this' and storing it in class member + std::function Captured; + // CHECK-MESSAGES: :[[@LINE-1]]:25: note: 'std::function' that stores captured 'this' +}; + +struct AssignCapture { + AssignCapture() : Captured([Self = this]() { static_cast(Self); }) {} + // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: using lambda expressions to capture 'this' and storing it in class member + std::function Captured; + // CHECK-MESSAGES: :[[@LINE-1]]:25: note: 'std::function' that stores captured 'this' +}; + +struct DeleteMoveAndCopy { + DeleteMoveAndCopy() : Captured([this]() { static_cast(this); }) {} + DeleteMoveAndCopy(DeleteMoveAndCopy const&) = delete; + DeleteMoveAndCopy(DeleteMoveAndCopy &&) = delete; + DeleteMoveAndCopy& operator=(DeleteMoveAndCopy const&) = delete; + DeleteMoveAndCopy& operator=(DeleteMoveAndCopy &&) = delete; + std::function Captured; +}; + +struct DeleteCopyImplicitDisabledMove { + DeleteCopyImplicitDisabledMove() : Captured([this]() { static_cast(this); }) {} + DeleteCopyImplicitDisabledMove(DeleteCopyImplicitDisabledMove const&) = delete; + DeleteCopyImplicitDisabledMove& operator=(DeleteCopyImplicitDisabledMove const&) = delete; denzor200 wrote: We don't really need to add boost headers at all, since noncopyable is simplest and tidy to implement by ourself: ``` namespace boost { class noncopyable { protected: noncopyable() {} ~noncopyable() {} private: noncopyable( const noncopyable& ); noncopyable& operator=( const noncopyable& ); }; } ``` https://github.com/llvm/llvm-project/pull/130297 ___ 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 new check bugprone-capture-this-by-field (PR #130297)
@@ -0,0 +1,44 @@ +//===--- CapturingThisByFieldCheck.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_CAPTURINGTHISBYFIELDCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CAPTURINGTHISBYFIELDCHECK_H + +#include "../ClangTidyCheck.h" +#include "clang/AST/ASTTypeTraits.h" +#include + +namespace clang::tidy::bugprone { + +/// Finds lambda captures that capture the ``this`` pointer and store it as +/// class members without handle the copy and move constructors and the +/// assignments. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/capturing-this-by-field.html +class CapturingThisByFieldCheck : public ClangTidyCheck { +public: + CapturingThisByFieldCheck(StringRef Name, ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + 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.CPlusPlus11; denzor200 wrote: Since this check doesn't cove binds, let's stay it as it is. https://github.com/llvm/llvm-project/pull/130297 ___ 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 new check bugprone-capture-this-by-field (PR #130297)
@@ -0,0 +1,144 @@ +// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-capturing-this-by-field %t -- -config="{CheckOptions: {bugprone-capturing-this-by-field.FunctionWrapperTypes: '::std::function;::Fn'}}" -- + +namespace std { + +template +class function; + +template +class function { +public: + function() noexcept; + template function(F &&); +}; + +} // namespace std + +struct Fn { + template Fn(F &&); +}; + +struct Basic { + Basic() : Captured([this]() { static_cast(this); }) {} + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: using lambda expressions to capture 'this' and storing it in class member + std::function Captured; + // CHECK-MESSAGES: :[[@LINE-1]]:25: note: 'std::function' that stores captured 'this' +}; + +struct AssignCapture { + AssignCapture() : Captured([Self = this]() { static_cast(Self); }) {} + // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: using lambda expressions to capture 'this' and storing it in class member + std::function Captured; + // CHECK-MESSAGES: :[[@LINE-1]]:25: note: 'std::function' that stores captured 'this' +}; + +struct DeleteMoveAndCopy { + DeleteMoveAndCopy() : Captured([this]() { static_cast(this); }) {} + DeleteMoveAndCopy(DeleteMoveAndCopy const&) = delete; + DeleteMoveAndCopy(DeleteMoveAndCopy &&) = delete; + DeleteMoveAndCopy& operator=(DeleteMoveAndCopy const&) = delete; + DeleteMoveAndCopy& operator=(DeleteMoveAndCopy &&) = delete; + std::function Captured; +}; + +struct DeleteCopyImplicitDisabledMove { + DeleteCopyImplicitDisabledMove() : Captured([this]() { static_cast(this); }) {} + DeleteCopyImplicitDisabledMove(DeleteCopyImplicitDisabledMove const&) = delete; + DeleteCopyImplicitDisabledMove& operator=(DeleteCopyImplicitDisabledMove const&) = delete; denzor200 wrote: That was C++03 way to implement `boost::noncopyable`, I think we should provide another test(possible in separate .cpp) with C++11 version of `boost::noncopyable`: ``` namespace boost { class noncopyable { protected: constexpr noncopyable() = default; ~noncopyable() = default; noncopyable( const noncopyable& ) = delete; noncopyable& operator=( const noncopyable& ) = delete; }; } ``` https://github.com/llvm/llvm-project/pull/130297 ___ 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 new check bugprone-capture-this-by-field (PR #130297)
https://github.com/denzor200 edited https://github.com/llvm/llvm-project/pull/130297 ___ 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 new check bugprone-capture-this-by-field (PR #130297)
@@ -0,0 +1,144 @@ +// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-capturing-this-by-field %t -- -config="{CheckOptions: {bugprone-capturing-this-by-field.FunctionWrapperTypes: '::std::function;::Fn'}}" -- denzor200 wrote: Created issues about bind and Boost's lambdas: https://github.com/llvm/llvm-project/issues/131220 https://github.com/llvm/llvm-project/issues/131231 https://github.com/llvm/llvm-project/pull/130297 ___ 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 new check bugprone-capture-this-by-field (PR #130297)
https://github.com/denzor200 edited https://github.com/llvm/llvm-project/pull/130297 ___ 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 new check bugprone-capture-this-by-field (PR #130297)
@@ -0,0 +1,44 @@ +//===--- CapturingThisByFieldCheck.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_CAPTURINGTHISBYFIELDCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_CAPTURINGTHISBYFIELDCHECK_H + +#include "../ClangTidyCheck.h" +#include "clang/AST/ASTTypeTraits.h" +#include + +namespace clang::tidy::bugprone { + +/// Finds lambda captures that capture the ``this`` pointer and store it as +/// class members without handle the copy and move constructors and the +/// assignments. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone/capturing-this-by-field.html +class CapturingThisByFieldCheck : public ClangTidyCheck { +public: + CapturingThisByFieldCheck(StringRef Name, ClangTidyContext *Context); + void storeOptions(ClangTidyOptions::OptionMap &Opts) override; + 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.CPlusPlus11; denzor200 wrote: Agree about lambda, but in C++03 mode we could use bind instead of lambda to reproduce the same problem. https://github.com/llvm/llvm-project/pull/130297 ___ 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 new check bugprone-capture-this-by-field (PR #130297)
@@ -0,0 +1,144 @@ +// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-capturing-this-by-field %t -- -config="{CheckOptions: {bugprone-capturing-this-by-field.FunctionWrapperTypes: '::std::function;::Fn'}}" -- denzor200 wrote: Oke, don't worry about bind and Boost's lambdas. I will create issues about all of them later. https://github.com/llvm/llvm-project/pull/130297 ___ 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 new check bugprone-capture-this-by-field (PR #130297)
denzor200 wrote: LGTM, please provide the change for one comment I left https://github.com/llvm/llvm-project/pull/130297 ___ 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 new check bugprone-capture-this-by-field (PR #130297)
https://github.com/denzor200 approved this pull request. https://github.com/llvm/llvm-project/pull/130297 ___ 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 new check bugprone-capture-this-by-field (PR #130297)
@@ -0,0 +1,36 @@ +.. title:: clang-tidy - bugprone-capturing-this-in-member-variable + +bugprone-capturing-this-in-member-variable +== + +Finds lambda captures that capture the ``this`` pointer and store it as class +members without handle the copy and move constructors and the assignments. + +Capture this in a lambda and store it as a class member is dangerous because the +lambda can outlive the object it captures. Especially when the object is copied +or moved, the captured ``this`` pointer will be implicitly propagated to the +new object. Most of the time, people will believe that the captured ``this`` +pointer points to the new object, which will lead to bugs. + +.. code-block:: c++ + + struct C { +C() : Captured([this]() -> C const * { return this; }) {} +std::function Captured; + }; + + void foo() { +C v1{}; +C v2 = v1; // v2.Captured capture v1's 'this' pointer +assert(v2.Captured() == v1.Captured()); // v2.Captured capture v1's 'this' pointer +assert(v2.Captured() == &v2); // assertion failed. + } + +Possible fixes include refactoring the function object into a class member denzor200 wrote: And the rest of the users who really want the class to be copyable will refactor the code in the way you suggested. https://github.com/llvm/llvm-project/pull/130297 ___ 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 new check bugprone-capture-this-by-field (PR #130297)
@@ -0,0 +1,36 @@ +.. title:: clang-tidy - bugprone-capturing-this-in-member-variable + +bugprone-capturing-this-in-member-variable +== + +Finds lambda captures that capture the ``this`` pointer and store it as class +members without handle the copy and move constructors and the assignments. + +Capture this in a lambda and store it as a class member is dangerous because the +lambda can outlive the object it captures. Especially when the object is copied +or moved, the captured ``this`` pointer will be implicitly propagated to the +new object. Most of the time, people will believe that the captured ``this`` +pointer points to the new object, which will lead to bugs. + +.. code-block:: c++ + + struct C { +C() : Captured([this]() -> C const * { return this; }) {} +std::function Captured; + }; + + void foo() { +C v1{}; +C v2 = v1; // v2.Captured capture v1's 'this' pointer +assert(v2.Captured() == v1.Captured()); // v2.Captured capture v1's 'this' pointer +assert(v2.Captured() == &v2); // assertion failed. + } + +Possible fixes include refactoring the function object into a class member denzor200 wrote: The first possible fix is to make the class completely uncopyable. In 90% of cases, the real reason for this warning is that someone forgot to disallow copying. https://github.com/llvm/llvm-project/pull/130297 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang-tools-extra] [clang-tidy] Improve `bugprone-capturing-this-in-member-variable` check: add support of `bind` functions. (PR #132635)
@@ -64,16 +64,21 @@ AST_MATCHER(CXXRecordDecl, correctHandleCaptureThisLambda) { constexpr const char *DefaultFunctionWrapperTypes = "::std::function;::std::move_only_function;::boost::function"; +constexpr const char *DefaultBindFunctions = "::std::bind;::boost::bind"; denzor200 wrote: Please add `::std::bind_front` and `::std::bind_back`: https://en.cppreference.com/w/cpp/utility/functional/bind_front There are also Boost analogue for them - `::boost::compat::bind_front` and `::boost::compat::bind_back`: https://www.boost.org/doc/libs/1_86_0/libs/compat/doc/html/compat.html#bind_front https://www.boost.org/doc/libs/1_86_0/libs/compat/doc/html/compat.html#bind_back https://github.com/llvm/llvm-project/pull/132635 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits