diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureThisWithCaptureDefaultCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureThisWithCaptureDefaultCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureThisWithCaptureDefaultCheck.h @@ -0,0 +1,45 @@ +//===--- AvoidCaptureThisWithCaptureDefaultCheck.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_AVOIDCAPTURETHISWITHCAPTUREDEFAULTCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDCAPTURETHISWITHCAPTUREDEFAULTCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace 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-this-with-capture-default.html +class AvoidCaptureThisWithCaptureDefaultCheck : public ClangTidyCheck { +public: + AvoidCaptureThisWithCaptureDefaultCheck(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 cppcoreguidelines +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDCAPTURETHISWITHCAPTUREDEFAULTCHECK_H diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureThisWithCaptureDefaultCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureThisWithCaptureDefaultCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureThisWithCaptureDefaultCheck.cpp @@ -0,0 +1,96 @@ +//===--- AvoidCaptureThisWithCaptureDefaultCheck.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 "AvoidCaptureThisWithCaptureDefaultCheck.h" +#include "../utils/LexerUtils.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "llvm/Support/raw_ostream.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace cppcoreguidelines { + +void AvoidCaptureThisWithCaptureDefaultCheck::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) { + 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 AvoidCaptureThisWithCaptureDefaultCheck::check( + const MatchFinder::MatchResult &Result) { + if (const auto *Lambda = Result.Nodes.getNodeAs("lambda")) { + if (Lambda->getCaptureDefault() != LCD_None) { + auto Diag = diag( + Lambda->getCaptureDefaultLoc(), + "lambdas that capture 'this' should not specify a capture default"); + + std::string ReplacementText = createReplacementText(Lambda); + SourceLocation DefaultCaptureEnd = + findDefaultCaptureEnd(Lambda, *Result.Context); + Diag << FixItHint::CreateReplacement( + CharSourceRange::getCharRange(Lambda->getCaptureDefaultLoc(), + DefaultCaptureEnd), + ReplacementText); + } + } +} + +} // namespace cppcoreguidelines +} // namespace tidy +} // namespace clang 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 + AvoidCaptureThisWithCaptureDefaultCheck.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 "AvoidCaptureThisWithCaptureDefaultCheck.h" #include "AvoidConstOrRefDataMembersCheck.h" #include "AvoidDoWhileCheck.h" #include "AvoidGotoCheck.h" @@ -50,6 +51,8 @@ void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { CheckFactories.registerCheck( "cppcoreguidelines-avoid-c-arrays"); + CheckFactories.registerCheck( + "cppcoreguidelines-avoid-capture-this-with-capture-default"); CheckFactories.registerCheck( "cppcoreguidelines-avoid-const-or-ref-data-members"); 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 @@ -111,6 +111,12 @@ 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-this-with-capture-default + ` check. + + Warns when lambda specify a capture default and capture ``this``. The check also + offers fix-its. + - New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capture-this-with-capture-default.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capture-this-with-capture-default.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capture-this-with-capture-default.rst @@ -0,0 +1,44 @@ +.. title:: clang-tidy - cppcoreguidelines-avoid-capture-this-with-capture-default + +cppcoreguidelines-avoid-capture-this-with-capture-default +========================================================= + +Warns when lambda specify a capture default and capture ``this``. The check also +offers fix-its. + +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 DataMember; + void misleadingLogic() { + int local = 0; + member = 0; + auto f = [=]() { + local += 1; + member += 1; + }; + f(); + // Here, local is 0 but member is 1 + } + + void clearLogic() { + int local = 0; + member = 0; + auto f = [this, local]() { + 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-this-with-capture-default `_, "Yes" `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-this-with-capture-default.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-this-with-capture-default.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-this-with-capture-default.cpp @@ -0,0 +1,92 @@ +// RUN: %check_clang_tidy -std=c++11-or-later %s cppcoreguidelines-avoid-capture-this-with-capture-default %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-this-with-capture-default] + // CHECK-FIXES: [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-this-with-capture-default] + // CHECK-FIXES: [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-this-with-capture-default] + // CHECK-FIXES: [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-this-with-capture-default] + // CHECK-FIXES: [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-this-with-capture-default] + // CHECK-FIXES: [&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-this-with-capture-default] + // CHECK-FIXES: [&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-this-with-capture-default] + // CHECK-FIXES: [&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-this-with-capture-default] + // CHECK-FIXES: [& 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-this-with-capture-default] + // CHECK-FIXES: [& /* byref */ local, &local2, this]() { return (local+x) > 10; } + + auto implicit_this_capture = [=]() { return x > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-this-with-capture-default] + // CHECK-FIXES: [this]() { return x > 10; }; + + auto implicit_this_capture_local = [=]() { return (local+x) > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:41: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-this-with-capture-default] + // CHECK-FIXES: [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-this-with-capture-default] + // CHECK-FIXES: [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-this-with-capture-default] + // CHECK-FIXES: [&local, this]() { return (local+x) > 10; }; + + auto ref_implicit_this_capture = [&]() { return x > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-this-with-capture-default] + // CHECK-FIXES: [this]() { return x > 10; }; + + auto ref_implicit_this_capture_local = [&]() { return (local+x) > 10; }; + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-this-with-capture-default] + // CHECK-FIXES: [&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 capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-this-with-capture-default] + // CHECK-FIXES: [&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; +};