PiotrZSL accepted this revision.
PiotrZSL added a comment.
This revision is now accepted and ready to land.
Just some minor issues in documentation.
To be honest for me this check still looks too strict, for example it will warn
when we capture `this` explicitly, and we don't use any class fields, but we
use some local variables captured by value and for example call some other
class method using that this pointer.
But well, i'm not fan of cppcoreguidelines, I justt fell that most of rules
there are not 100% implementable in a normal project.
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidValueCaptureDefaultThisCheck.h:17-25
+/// Warns when lambda specify a by-value capture default and capture ``this``.
+/// The check also offers fix-its.
///
-/// Capture defaults in lambas defined within member functions can be
+/// By-value capture defaults in lambas defined within member functions can be
/// misleading about whether capturing data member is by value or reference.
/// For example, [=] will capture local variables by value but member variables
+/// by reference. CppCoreGuideline F.54 suggests to never use by-value capture
----------------
thats too much information for an check short description.
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp:55
+ AvoidByValueCaptureDefaultWhenCapturingThisCheck>(
+
"cppcoreguidelines-avoid-by-value-capture-default-when-capturing-this");
CheckFactories.registerCheck<AvoidCapturingLambdaCoroutinesCheck>(
----------------
carlosgalvezp wrote:
> PiotrZSL wrote:
> > PiotrZSL wrote:
> > > this name is hard to understand
> > >
> > > I asked ChatGPT about it, and here are some other proposals:
> > >
> > > - cppcoreguidelines-avoid-by-value-default-this-capture
> > > - cppcoreguidelines-avoid-this-capture-by-value-default
> > > - cppcoreguidelines-explicit-this-capture-by-value
> > > - cppcoreguidelines-implicit-this-capture-by-value
> > > - cppcoreguidelines-implicit-by-value-this-capture
> > > - cppcoreguidelines-prefer-explicit-this-capture
> > > - cppcoreguidelines-avoid-ambiguous-this-capture
> > Probably something like this would be good:
> > cppcoreguidelines-avoid-implicit-this-capture-by-value
> Nice suggestions! I've done a bit of mix of them, let me know if you are
> happy with it.
To be honest that "default" sounds strange. Simply I check name does not read
as an "sentence", more as bunch of words, but I'm fine with this.
================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:118
Warns when lambda specify a capture default and capture ``this``.
----------------
maybe "Warns when a lambda captures `this` using the default by-value capture."
and align this with check doxygen comment and documentation, because you
changed those two but not here.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-value-capture-default-this.rst:11
specifying the capture default ``[=]`` will still capture data members
by reference.
----------------
Note: I'm missing here explanation why. (yes I know that this may not be
related)
Maybe:
"By-value capture-defaults in member functions can be misleading about whether
data members are captured by value or reference. This occurs because specifying
the capture default [=] actually captures the this pointer by value, not the
data members themselves. As a result, data members are still indirectly
accessed via the captured this pointer, which essentially means they are being
accessed by reference. Therefore, even when using [=], data members are
effectively captured by reference, which might not align with the user's
expectations."
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148340/new/
https://reviews.llvm.org/D148340
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
