[PATCH] D148340: [clang-tidy] Apply cppcoreguidelines-avoid-capture-default-when-capturin-this only to by-value capture default

[Piotr Zegar via Phabricator via cfe-commits]

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