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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
