ccotter created this revision.
Herald added subscribers: carlosgalvezp, shchenz, kbarton, xazax.hun, nemanjai.
Herald added a reviewer: njames93.
Herald added a project: All.
ccotter requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Warn when a lambda specifies a default capture and captures
``this``. Offer FixIts to correct the code.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D141133
Files:
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureThisWithCaptureDefaultCheck.cpp
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureThisWithCaptureDefaultCheck.h
clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capture-this-with-capture-default.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-this-with-capture-default.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-this-with-capture-default.cpp
===================================================================
--- /dev/null
+++ 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;
+};
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -178,6 +178,7 @@
`clang-analyzer-valist.Unterminated <clang-analyzer/valist.Unterminated.html>`_,
`concurrency-mt-unsafe <concurrency/mt-unsafe.html>`_,
`concurrency-thread-canceltype-asynchronous <concurrency/thread-canceltype-asynchronous.html>`_,
+ `cppcoreguidelines-avoid-capture-this-with-capture-default <cppcoreguidelines/avoid-capture-this-with-capture-default.html>`_, "Yes"
`cppcoreguidelines-avoid-const-or-ref-data-members <cppcoreguidelines/avoid-const-or-ref-data-members.html>`_,
`cppcoreguidelines-avoid-do-while <cppcoreguidelines/avoid-do-while.html>`_,
`cppcoreguidelines-avoid-goto <cppcoreguidelines/avoid-goto.html>`_,
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capture-this-with-capture-default.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capture-this-with-capture-default.rst
@@ -0,0 +1,41 @@
+.. title:: clang-tidy - cppcoreguidelines-avoid-capture-this-with-capture-default
+
+cppcoreguidelines-avoid-capture-this-with-capture-default
+=========================================================
+
+Default lambda captures in member functions can be misleading about
+whether data members are captured by value or reference. For example,
+specifying the default capture ``[=]`` 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 <http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f54-if-you-capture-this-capture-all-variables-explicitly-no-default-capture>`_.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -108,6 +108,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
+ <clang-tidy/checks/cppcoreguidelines/avoid-capture-this-with-capture-default>` check.
+
+ Warns when lambda specify a capture default and capture ``this``. The check also
+ offers FixIts.
+
- New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
<clang-tidy/checks/cppcoreguidelines/avoid-const-or-ref-data-members>` check.
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ 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"
@@ -49,6 +50,8 @@
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
CheckFactories.registerCheck<modernize::AvoidCArraysCheck>(
"cppcoreguidelines-avoid-c-arrays");
+ CheckFactories.registerCheck<AvoidCaptureThisWithCaptureDefaultCheck>(
+ "cppcoreguidelines-avoid-capture-this-with-capture-default");
CheckFactories.registerCheck<AvoidConstOrRefDataMembersCheck>(
"cppcoreguidelines-avoid-const-or-ref-data-members");
CheckFactories.registerCheck<AvoidDoWhileCheck>(
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
@@ -4,6 +4,7 @@
)
add_clang_library(clangTidyCppCoreGuidelinesModule
+ AvoidCaptureThisWithCaptureDefaultCheck.cpp
AvoidConstOrRefDataMembersCheck.cpp
AvoidDoWhileCheck.cpp
AvoidGotoCheck.cpp
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureThisWithCaptureDefaultCheck.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureThisWithCaptureDefaultCheck.h
@@ -0,0 +1,42 @@
+//===--- 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 {
+
+/// Default lambda captures in 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 default capture 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
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureThisWithCaptureDefaultCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureThisWithCaptureDefaultCheck.cpp
@@ -0,0 +1,100 @@
+//===--- 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);
+ llvm::errs() << "FOR REF capture loc= "
+ << Capture.getLocation().printToString(SourceMgr)
+ << " addr=" << AddressofLoc.printToString(SourceMgr)
+ << "\n";
+ 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<LambdaExpr>("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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
