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:
> ccotter wrote:
> > ccotter wrote:
> > > 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
> > https://github.com/isocpp/CppCoreGuidelines/pull/2016 for "capture default"
> > vs "default capture"
> 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!
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."
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D141133/new/
https://reviews.llvm.org/D141133
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
