[PATCH] D141133: [clang-tidy] Implement CppCoreGuideline F.54

[Chris Cotter via Phabricator via cfe-commits]

ccotter marked an inline comment as done.
ccotter added inline comments.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-this-with-capture-default.cpp:9
+    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]() { };
----------------
carlosgalvezp wrote:
> carlosgalvezp wrote:
> > carlosgalvezp wrote:
> > > "default capture"?
> > 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.
> 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.
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
```


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-this-with-capture-default.cpp:9
+    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]() { };
----------------
ccotter wrote:
> carlosgalvezp wrote:
> > carlosgalvezp wrote:
> > > carlosgalvezp wrote:
> > > > "default capture"?
> > > 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.
> > 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.
> 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
> ```
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.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-this-with-capture-default.cpp:9
+    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]() { };
----------------
ccotter wrote:
> ccotter wrote:
> > carlosgalvezp wrote:
> > > carlosgalvezp wrote:
> > > > carlosgalvezp wrote:
> > > > > "default capture"?
> > > > 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.
> > > 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.
> > 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
> > ```
> 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.
+1 - I admit, I struggled naming this check. More feedback welcome on the name


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141133/new/

https://reviews.llvm.org/D141133