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

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureThisWithCaptureDefaultCheck.cpp:35
+            Capture.getLocation(), SourceMgr, Context.getLangOpts(), tok::amp);
+        llvm::errs() << "FOR REF capture loc= "
+                     << Capture.getLocation().printToString(SourceMgr)
----------------
Not having being involved in the development of this check I don't quite understand what this error message means, could you provide a more descriptive message?

It's also unclear if the program is supposed to abort when entering this branch, or if it's expected that it returns just fine? If it's supposed to return just fine, I think the check should not print anything, to keep the users' logs clean.


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:114
+
+  Warns when lambda specify a capture default and capture ``this``. The check also
+  offers fix-its.
----------------
This text should also be at the beginning of the check class documentation (in the Check.h, line 18)


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capture-this-with-capture-default.rst:9
+
+Capture-deafults in member functions can be misleading about
+whether data members are captured by value or reference. For example,
----------------
defaults


================
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:
> > 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."
Another suggestion for the check name: 

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




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141133