diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.h @@ -0,0 +1,41 @@ +//===--- AvoidCaptureDefaultWhenCapturingThisCheck.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_CPPCOREGUIDELINES_AVOIDCAPTUREDEFAULTWHENCAPTURINGTHISCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDCAPTUREDEFAULTWHENCAPTURINGTHISCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::cppcoreguidelines { + +/// Warns when lambda specify a capture default and capture ``this``. The check +/// also offers fix-its. +/// +/// Capture defaults in lambas defined within member functions can be +/// misleading about whether capturing data member is by value or reference. +/// For example, [=] will capture local variables by value but member variables +/// by reference. CppCoreGuideline F.54 suggests to always be explicit +/// and never specify a capture default when also capturing this. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/avoid-capture-default-when-capturing-this.html +class AvoidCaptureDefaultWhenCapturingThisCheck : public ClangTidyCheck { +public: + AvoidCaptureDefaultWhenCapturingThisCheck(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.CPlusPlus11; + } +}; + +} // namespace clang::tidy::cppcoreguidelines + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDCAPTUREDEFAULTWHENCAPTURINGTHISCHECK_H diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.cpp @@ -0,0 +1,98 @@ +//===--- AvoidCaptureDefaultWhenCapturingThisCheck.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 "AvoidCaptureDefaultWhenCapturingThisCheck.h" +#include "../utils/LexerUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "llvm/Support/raw_ostream.h" + +#include + +using namespace clang::ast_matchers; + +namespace clang::tidy::cppcoreguidelines { + +void AvoidCaptureDefaultWhenCapturingThisCheck::registerMatchers( + MatchFinder *Finder) { + Finder->addMatcher(lambdaExpr(hasAnyCapture(capturesThis())).bind("lambda"), + this); +} + +static SourceLocation findDefaultCaptureEnd(const LambdaExpr *Lambda, + ASTContext &Context) { + for (const LambdaCapture &Capture : Lambda->explicit_captures()) { + if (Capture.isExplicit()) { + if (Capture.getCaptureKind() == LCK_ByRef) { + const SourceManager &SourceMgr = Context.getSourceManager(); + SourceLocation AddressofLoc = utils::lexer::findPreviousTokenKind( + Capture.getLocation(), SourceMgr, Context.getLangOpts(), tok::amp); + return AddressofLoc; + } else { + return Capture.getLocation(); + } + } + } + return Lambda->getIntroducerRange().getEnd(); +} + +static std::string createReplacementText(const LambdaExpr *Lambda) { + std::string Replacement; + llvm::raw_string_ostream Stream(Replacement); + + auto AppendName = [&](llvm::StringRef Name) { + if (Replacement.size() != 0) { + Stream << ", "; + } + if (Lambda->getCaptureDefault() == LCD_ByRef && Name != "this") { + Stream << "&" << Name; + } else { + Stream << Name; + } + }; + + for (const LambdaCapture &Capture : Lambda->implicit_captures()) { + assert(Capture.isImplicit()); + if (Capture.capturesVariable() && Capture.isImplicit()) { + AppendName(Capture.getCapturedVar()->getName()); + } else if (Capture.capturesThis()) { + AppendName("this"); + } + } + if (Replacement.size() && + Lambda->explicit_capture_begin() != Lambda->explicit_capture_end()) { + // Add back separator if we are adding explicit capture variables. + Stream << ", "; + } + return Replacement; +} + +void AvoidCaptureDefaultWhenCapturingThisCheck::check( + const MatchFinder::MatchResult &Result) { + if (const auto *Lambda = Result.Nodes.getNodeAs("lambda")) { + if (Lambda->getCaptureDefault() != LCD_None) { + bool IsThisImplicitlyCaptured = std::any_of( + Lambda->implicit_capture_begin(), Lambda->implicit_capture_end(), + [](const LambdaCapture &Capture) { return Capture.capturesThis(); }); + auto Diag = diag(Lambda->getCaptureDefaultLoc(), + "lambdas that %select{|implicitly }0capture 'this' " + "should not specify a capture default") + << IsThisImplicitlyCaptured; + + std::string ReplacementText = createReplacementText(Lambda); + SourceLocation DefaultCaptureEnd = + findDefaultCaptureEnd(Lambda, *Result.Context); + Diag << FixItHint::CreateReplacement( + CharSourceRange::getCharRange(Lambda->getCaptureDefaultLoc(), + DefaultCaptureEnd), + ReplacementText); + } + } +} + +} // namespace clang::tidy::cppcoreguidelines diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt @@ -4,6 +4,7 @@ ) add_clang_library(clangTidyCppCoreGuidelinesModule + AvoidCaptureDefaultWhenCapturingThisCheck.cpp AvoidConstOrRefDataMembersCheck.cpp AvoidDoWhileCheck.cpp AvoidGotoCheck.cpp diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp @@ -14,6 +14,7 @@ #include "../modernize/AvoidCArraysCheck.h" #include "../modernize/UseOverrideCheck.h" #include "../readability/MagicNumbersCheck.h" +#include "AvoidCaptureDefaultWhenCapturingThisCheck.h" #include "AvoidConstOrRefDataMembersCheck.h" #include "AvoidDoWhileCheck.h" #include "AvoidGotoCheck.h" @@ -47,6 +48,8 @@ class CppCoreGuidelinesModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck( + "cppcoreguidelines-avoid-capture-default-when-capturing-this"); CheckFactories.registerCheck( "cppcoreguidelines-avoid-c-arrays"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -115,6 +115,11 @@ Finds usages of ``realloc`` where the return value is assigned to the same expression as passed to the first argument. +- New :doc:`cppcoreguidelines-avoid-capture-default-when-capturing-this + ` check. + + Warns when lambda specify a capture default and capture ``this``. + - New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capture-default-when-capturing-this.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capture-default-when-capturing-this.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capture-default-when-capturing-this.rst @@ -0,0 +1,43 @@ +.. title:: clang-tidy - cppcoreguidelines-avoid-capture-default-when-capturing-this + +cppcoreguidelines-avoid-capture-default-when-capturing-this +=========================================================== + +Warns when lambda specify a capture default and capture ``this``. + +Capture-defaults in member functions can be misleading about +whether data members are captured by value or reference. For example, +specifying the capture default ``[=]`` will still capture data members +by reference. + +Examples: + +.. code-block:: c++ + + struct AClass { + int member; + void misleadingLogic() { + int local = 0; + member = 0; + auto f = [=]() mutable { + local += 1; + member += 1; + }; + f(); + // Here, local is 0 but member is 1 + } + + void clearLogic() { + int local = 0; + member = 0; + auto f = [this, local]() mutable { + local += 1; + member += 1; + }; + f(); + // Here, local is 0 but member is 1 + } + }; + +This check implements +`CppCoreGuideline F.54 `_. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -178,6 +178,7 @@ `clang-analyzer-valist.Unterminated `_, `concurrency-mt-unsafe `_, `concurrency-thread-canceltype-asynchronous `_, + `cppcoreguidelines-avoid-capture-default-when-capturing-this `_, `cppcoreguidelines-avoid-const-or-ref-data-members `_, `cppcoreguidelines-avoid-do-while `_, `cppcoreguidelines-avoid-goto `_, diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-default-when-capturing-this.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-default-when-capturing-this.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-default-when-capturing-this.cpp @@ -0,0 +1,92 @@ +// RUN: %check_clang_tidy -std=c++11-or-later %s cppcoreguidelines-avoid-capture-default-when-capturing-this %t + +struct Obj { + void lambdas_that_warn_default_capture_copy() { + int local{}; + int local2{}; + + auto explicit_this_capture = [=, this]() { }; + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this] + // CHECK-FIXES: auto explicit_this_capture = [this]() { }; + + auto explicit_this_capture_locals1 = [=, this]() { return (local+x) > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:43: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this] + // CHECK-FIXES: auto explicit_this_capture_locals1 = [local, this]() { return (local+x) > 10; }; + + auto explicit_this_capture_locals2 = [=, this]() { return (local+local2) > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:43: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this] + // CHECK-FIXES: auto explicit_this_capture_locals2 = [local, local2, this]() { return (local+local2) > 10; }; + + auto explicit_this_capture_local_ref = [=, this, &local]() { return (local+x) > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this] + // CHECK-FIXES: auto explicit_this_capture_local_ref = [this, &local]() { return (local+x) > 10; }; + + auto explicit_this_capture_local_ref2 = [=, &local, this]() { return (local+x) > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this] + // CHECK-FIXES: auto explicit_this_capture_local_ref2 = [&local, this]() { return (local+x) > 10; }; + + auto explicit_this_capture_local_ref3 = [=, &local, this, &local2]() { return (local+x) > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this] + // CHECK-FIXES: auto explicit_this_capture_local_ref3 = [&local, this, &local2]() { return (local+x) > 10; }; + + auto explicit_this_capture_local_ref4 = [=, &local, &local2, this]() { return (local+x) > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this] + // CHECK-FIXES: auto explicit_this_capture_local_ref4 = [&local, &local2, this]() { return (local+x) > 10; }; + + auto explicit_this_capture_local_ref_extra_whitespace = [=, & local, &local2, this]() { return (local+x) > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:62: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this] + // CHECK-FIXES: auto explicit_this_capture_local_ref_extra_whitespace = [& local, &local2, this]() { return (local+x) > 10; }; + + auto explicit_this_capture_local_ref_with_comment = [=, & /* byref */ local, &local2, this]() { return (local+x) > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:58: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this] + // CHECK-FIXES: auto explicit_this_capture_local_ref_with_comment = [& /* byref */ local, &local2, this]() { return (local+x) > 10; }; + + auto implicit_this_capture = [=]() { return x > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: lambdas that implicitly capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this] + // CHECK-FIXES: auto implicit_this_capture = [this]() { return x > 10; }; + + auto implicit_this_capture_local = [=]() { return (local+x) > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:41: warning: lambdas that implicitly capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this] + // CHECK-FIXES: auto implicit_this_capture_local = [local, this]() { return (local+x) > 10; }; + } + + void lambdas_that_warn_default_capture_ref() { + int local{}; + int local2{}; + + auto ref_explicit_this_capture = [&, this]() { }; + // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this] + // CHECK-FIXES: auto ref_explicit_this_capture = [this]() { }; + + auto ref_explicit_this_capture_local = [&, this]() { return (local+x) > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this] + // CHECK-FIXES: auto ref_explicit_this_capture_local = [&local, this]() { return (local+x) > 10; }; + + auto ref_implicit_this_capture = [&]() { return x > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: lambdas that implicitly capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this] + // CHECK-FIXES: auto ref_implicit_this_capture = [this]() { return x > 10; }; + + auto ref_implicit_this_capture_local = [&]() { return (local+x) > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: lambdas that implicitly capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this] + // CHECK-FIXES: auto ref_implicit_this_capture_local = [&local, this]() { return (local+x) > 10; }; + + auto ref_implicit_this_capture_locals = [&]() { return (local+local2+x) > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:46: warning: lambdas that implicitly capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this] + // CHECK-FIXES: auto ref_implicit_this_capture_locals = [&local, &local2, this]() { return (local+local2+x) > 10; }; + } + + void lambdas_that_dont_warn() { + int local{}; + int local2{}; + auto explicit_this_capture = [this]() { }; + auto explicit_this_capture_local = [this, local]() { return local > 10; }; + auto explicit_this_capture_local_ref = [this, &local]() { return local > 10; }; + + auto no_captures = []() {}; + auto capture_local_only = [local]() {}; + auto ref_capture_local_only = [&local]() {}; + auto capture_many = [local, &local2]() {}; + } + + int x; +};