[clang-tidy] Add option to ignore capture default by reference in cppcoreguidelines-avoid-capture-default-when-capturing-this

[carlosgalvezp]

Summary

The rule exists primarily for when using capture default
by copy "[=]", since member variables will be captured by
reference, which is against developer expectations.

However when the capture default is by reference, then there
is no doubt: everything will be captured by reference. Add
an option to allow just that.

Note: Release Notes do not need update since this check
has been introduced in the current WIP release.

A ticket has been opened at the C++ Core Guidelines repo
to consider updating the rule such that this behavior
is the default one:
https://github.com/isocpp/CppCoreGuidelines/issues/2060

[carlosgalvezp]

Clean up check suffix in test.

[carlosgalvezp]

Fix typo in doc.

[PiotrZSL]

No comments from me, looks ok.

[ccotter]

If https://github.com/isocpp/CppCoreGuidelines/issues/2060 is accepted to only consider [=], then I assume we'd want to change the default value of IgnoreCaptureDefaultByReference to be true? Thanks by the way for this!

[carlosgalvezp]

In D147062#4228403, @ccotter wrote:

If https://github.com/isocpp/CppCoreGuidelines/issues/2060 is accepted to only consider [=], then I assume we'd want to change the default value of IgnoreCaptureDefaultByReference to be true? Thanks by the way for this!

Thanks for reviewing! If they do accept it (I'm not very optimistic) then I'd probably just remove this logic and exit early if the lambda capture is not LCD_ByCopy.

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.h

 /// 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) {}
+                                            ClangTidyContext *Context);
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
   bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
     return LangOpts.CPlusPlus11;
   }
+
+private:
+  bool IgnoreCaptureDefaultByReference;
 };
 
 } // namespace clang::tidy::cppcoreguidelines
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_AVOIDCAPTUREDEFAULTWHENCAPTURINGTHISCHECK_H

clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.cpp

 #include "llvm/Support/raw_ostream.h"
 
 #include <algorithm>
 
 using namespace clang::ast_matchers;
 
 namespace clang::tidy::cppcoreguidelines {
 
+AvoidCaptureDefaultWhenCapturingThisCheck::
+    AvoidCaptureDefaultWhenCapturingThisCheck(StringRef Name,
+                                              ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      IgnoreCaptureDefaultByReference(
+          Options.get("IgnoreCaptureDefaultByReference", false)) {}
+
+void AvoidCaptureDefaultWhenCapturingThisCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "IgnoreCaptureDefaultByReference",
+                IgnoreCaptureDefaultByReference);
+}
+
 void AvoidCaptureDefaultWhenCapturingThisCheck::registerMatchers(
     MatchFinder *Finder) {
   Finder->addMatcher(lambdaExpr(hasAnyCapture(capturesThis())).bind("lambda"),
                      this);
 }
 
 static SourceLocation findDefaultCaptureEnd(const LambdaExpr *Lambda,
                                             ASTContext &Context) {
if (Replacement.size() &&
     // 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<LambdaExpr>("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;
+  const auto *Lambda = Result.Nodes.getNodeAs<LambdaExpr>("lambda");
+  if (!Lambda)
+    return;
+
+  if (IgnoreCaptureDefaultByReference &&
+      Lambda->getCaptureDefault() == LCD_ByRef)
+    return;
+
 
-      std::string ReplacementText = createReplacementText(Lambda);
-      SourceLocation DefaultCaptureEnd =
-          findDefaultCaptureEnd(Lambda, *Result.Context);
-      Diag << FixItHint::CreateReplacement(
-          CharSourceRange::getCharRange(Lambda->getCaptureDefaultLoc(),
-                                        DefaultCaptureEnd),
-          ReplacementText);
-    }
-  }
-}
 
 } // namespace clang::tidy::cppcoreguidelines

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capture-default-when-capturing-this.rst

.. code-block:: c++
           };
           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>`_.
+
+
+Options
+-------
+
+.. option:: IgnoreCaptureDefaultByReference
+
+  Do not warn when using capture default by reference. In this case, there is no
+  confusion as to whether variables are captured by value or reference.
+  Defaults to `false`.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-default-when-capturing-this.cpp

-// RUN: %check_clang_tidy -std=c++11-or-later %s cppcoreguidelines-avoid-capture-default-when-capturing-this %t
+// RUN: %check_clang_tidy -std=c++11-or-later %s cppcoreguidelines-avoid-capture-default-when-capturing-this %t \
+// RUN: -check-suffixes=,DEFAULT
+// RUN: %check_clang_tidy -std=c++11-or-later %s cppcoreguidelines-avoid-capture-default-when-capturing-this %t \
+// RUN: -config="{CheckOptions: [{key: cppcoreguidelines-avoid-capture-default-when-capturing-this.IgnoreCaptureDefaultByReference, value: true}]}"
 
 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]
void lambdas_that_warn_default_capture_copy() {
     // 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]() { };
+    // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:39: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES-DEFAULT: 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; };
+    // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:45: warning: lambdas that capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES-DEFAULT: 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; };
+    // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:39: warning: lambdas that implicitly capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES-DEFAULT: 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; };
+    // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:45: warning: lambdas that implicitly capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES-DEFAULT: 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; };
+    // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:46: warning: lambdas that implicitly capture 'this' should not specify a capture default [cppcoreguidelines-avoid-capture-default-when-capturing-this]
+    // CHECK-FIXES-DEFAULT: 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; };