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

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp 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]() { };
----------------
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!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141133