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