Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Differential D141133 Diff 487084 clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-this-with-capture-default.cpp

Changeset View

Standalone View

View Options

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

  • This file was added.
// RUN: %check_clang_tidy -std=c++11-or-later %s cppcoreguidelines-avoid-capture-this-with-capture-default %t
Lint: Lint
Inline Actions

clang-format not found in user’s local PATH; not linting file.

Lint: Lint: clang-format not found in user’s local PATH; not linting file.
carlosgalvezpUnsubmitted
Not Done
ReplyInline Actions

I believe this is the default, does it make sense to remove it or do we need to be explicit?

carlosgalvezp: I believe this is the default, does it make sense to remove it or do we need to be explicit?
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]() { };
carlosgalvezpUnsubmitted
Not Done
ReplyInline Actions

"default capture"?

carlosgalvezp: "default capture"?
carlosgalvezpUnsubmitted
Not Done
ReplyInline Actions

I find the check name a bit unintuitive. If you are up for a rename (you can use rename_check.py), I would consider renaming to something like cppcoreguidelines-avoid-default-capture-when-capturing-this

Like, what should be avoided is not "capturing this", it's using a default capture.

Would be good to get other reviewers opinion before spending time on renaming.

carlosgalvezp: I find the check name a bit unintuitive. If you are up for a rename (you can use `rename_check.
carlosgalvezpUnsubmitted
Done
ReplyInline Actions

Maybe put it within quotes so clarify it's a C++ keyword? Either backticks this or single quotes 'this' would work I think, unless we have some other convention.

carlosgalvezp: Maybe put it within quotes so clarify it's a C++ keyword? Either backticks `this` or single…
ccotterAuthorUnsubmitted
Not Done
ReplyInline Actions

I updated all references to capture default to say "capture default" as this is how it is spelled in the standard. The CppCoreGuideline for F.54 does use "default capture" though, so I'll open an issue seeing if that wording should instead say "capture default." Also for reference, git grep within the llvm-project repo shows

$ git grep -i 'capture default' | wc -l
      43
$ git grep -i 'default capture' | wc -l
      54
$ git grep -i 'capture.default' | wc -l # E.g., 'capture-default' or 'capture default'
     105
ccotter: I updated all references to capture default to say "capture default" as this is how it is…
ccotterAuthorUnsubmitted
Not Done
ReplyInline Actions

+1 - I admit, I struggled naming this check. More feedback welcome on the name

ccotter: +1 - I admit, I struggled naming this check. More feedback welcome on the name
ccotterAuthorUnsubmitted
Done
ReplyInline Actions

Updated with a single quote. I didn't find any clang-tidy diagnostics emitting backticks, but saw usage of single quotes when referring to identifiers like variable or class names.

ccotter: Updated with a single quote. I didn't find any clang-tidy diagnostics emitting backticks, but…
ccotterAuthorUnsubmitted
Done
ReplyInline Actions

https://github.com/isocpp/CppCoreGuidelines/pull/2016 for "capture default" vs "default capture"

ccotter: https://github.com/isocpp/CppCoreGuidelines/pull/2016 for "capture default" vs "default capture"
carlosgalvezpUnsubmitted
Not Done
ReplyInline Actions

Interesting, thanks for investigating this! cppreference also uses that terminology.
It sounds quite strange to me since that's not how humans read english text (which is what the diagnostic is meant for). We use "default constructor", "default arguments", etc.

Anyhow this is a minor detail that shouldn't block the patch. Thanks for opening the pull requet towards CppCoreGuidelines!

carlosgalvezp: Interesting, thanks for investigating this! cppreference also uses that terminology. It sounds…
carlosgalvezpUnsubmitted
Not Done
ReplyInline Actions

Another suggestion for the check name:

"cppcoreguidelines-avoid-capture-default-in-member-functions"

carlosgalvezp: Another suggestion for the check name: "cppcoreguidelines-avoid-capture-default-in-member…
ccotterAuthorUnsubmitted
Done
ReplyInline Actions

Since this is a more minor detail, we could probably follow up on the "capture default" wording on the CppCoreGuidelines issue I opened. I'll add that I *just* changed the lone occurrence of "default capture" in https://en.cppreference.com/mwiki/index.php?title=cpp%2Flanguage%2Flambda&diff=146169&oldid=145543, since the other 15 references were phrased "capture default" or "capture-default."

ccotter: Since this is a more minor detail, we could probably follow up on the "capture default" wording…
njames93Unsubmitted
Not Done
ReplyInline Actions

Please can you include the whole expected line in the check fix markers, same goes for all tests below

njames93: Please can you include the whole expected line in the check fix markers, same goes for all…
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;
};