Warn when a lambda specifies a default capture and captures
`this`. Offer FixIts to correct the code.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-this-with-capture-default.cpp | ||
---|---|---|
9 ↗ | (On Diff #486873) | "default capture"? |
9 ↗ | (On Diff #486873) | 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. |
9 ↗ | (On Diff #486873) | 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. |
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-this-with-capture-default.cpp | ||
---|---|---|
1 ↗ | (On Diff #486873) | I believe this is the default, does it make sense to remove it or do we need to be explicit? |
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-this-with-capture-default.cpp | ||
---|---|---|
9 ↗ | (On Diff #486873) | 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 |
9 ↗ | (On Diff #486873) | 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. |
9 ↗ | (On Diff #486873) | +1 - I admit, I struggled naming this check. More feedback welcome on the name |
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-this-with-capture-default.cpp | ||
---|---|---|
9 ↗ | (On Diff #486873) | https://github.com/isocpp/CppCoreGuidelines/pull/2016 for "capture default" vs "default capture" |
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-this-with-capture-default.cpp | ||
---|---|---|
9 ↗ | (On Diff #486873) | Interesting, thanks for investigating this! cppreference also uses that terminology. Anyhow this is a minor detail that shouldn't block the patch. Thanks for opening the pull requet towards CppCoreGuidelines! |
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-this-with-capture-default.cpp | ||
---|---|---|
9 ↗ | (On Diff #486873) | 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." |
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureThisWithCaptureDefaultCheck.cpp | ||
---|---|---|
35 ↗ | (On Diff #487084) | 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. |
clang-tools-extra/docs/ReleaseNotes.rst | ||
103 | This text should also be at the beginning of the check class documentation (in the Check.h, line 18) | |
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capture-this-with-capture-default.rst | ||
9 ↗ | (On Diff #487084) | defaults |
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-this-with-capture-default.cpp | ||
9 ↗ | (On Diff #486873) | Another suggestion for the check name: "cppcoreguidelines-avoid-capture-default-in-member-functions" |
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureThisWithCaptureDefaultCheck.cpp | ||
---|---|---|
35 ↗ | (On Diff #487084) | My bad - I forgot to remove this trace. Both branches are valid branches for normal program execution. |
LGTM! Please give a few days for other reviewers to comment. If you are not 100% happy with the check name I'd suggest to change it before merging, it will be much harder to change once it's in.
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureThisWithCaptureDefaultCheck.cpp | ||
---|---|---|
32 ↗ | (On Diff #487130) | Should be const SourceManager &. |
Thanks!
I opted for 'avoid-capture-default-when-capturing-this' and updated the name and docs etc in the most recent diff.
LGTM! Minor nit.
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capture-default-when-capturing-this.rst | ||
---|---|---|
5 | Fix line so it's as long as the text above |
Can you run this through clang format to make sure the pre merge is happy and address those nits
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.cpp | ||
---|---|---|
82 | Would be nice to show if this is implicitly captured. | |
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/avoid-capture-default-when-capturing-this.rst | ||
7 | Not necessary to specify fix behaviour as that's provided in the check list | |
20 | Please fix these examples as this isn't actually captured because the names don't match. | |
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-this-with-capture-default.cpp | ||
10 ↗ | (On Diff #487195) | Please can you include the whole expected line in the check fix markers, same goes for all tests below |
Just 2 more CHECK-FIXES lines need updating.
FWIW the reason its good to be explicit is because FileCheck will start searching from the line where the previous CHECK-FIXES had and stop when it sees line that contains the match.
In this example:
// CHECK-FIXES: [this]() { };
If the there was an error with the fix, but later on in the file, that string was found. FileCheck would assume the CHECK-FIXES directive was found and report success.
That may in turn lead other CHECK-FIXES to fail due to the position of the previous match. But those error message would be slightly more confusing as the expected fix was actually found in the output file (just not in the correct place)
Expanding the check to this:
// CHECK-FIXES: auto explicit_this_capture = [this]() { };
Means we have to match the whole line(which is a lot less likely to appear again later in the file.)
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/avoid-capture-default-when-capturing-this.cpp | ||
---|---|---|
11 | ||
15 |
My latest update to the diff was the result of a rebase, then addressing the final comments. What I've been doing is something like
git pull upstream main --rebase # make more edits git commit -am .... arc diff --update D141133 upstream/main
I'm not sure if this is the best way to rebase phabs, but please let me know if there's a better way for phab to understand.
Would someone with merge access mind committing this? I double checked and this diff can be applied on the latest upstream/main. Thanks!
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.cpp | ||
---|---|---|
2 | Ditto. | |
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.h | ||
2 | Please merge these two lines. Amount of dashes and equal signs could be reduced. | |
clang-tools-extra/docs/ReleaseNotes.rst | ||
103 | The check also offers fix-its. could be omitted. This information presents in check table. |
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.cpp | ||
---|---|---|
20 | We recently switched to using C++17 nested namespaces for all the clang-tidy folder, please update to keep it consistent. |
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.cpp | ||
---|---|---|
20 | Just curious - I noticed the clang-tidy headers do not use nested namespaces as part of the recent switch. Are the headers slated to be updated too? |
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.h | ||
---|---|---|
42–44 | Forgot to comment but you'll need it here too :) |
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.cpp | ||
---|---|---|
20 | Oh crap, I must have missed to add the header-filter option when applying the change, it should apply to headers as well! Will fix in a separate patch. |
LGTM, thanks for the contribution! Let's give a couple days for the others to have a last look
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.cpp | ||
---|---|---|
82 | Was this comment addressed? I see it marked as "Not Done". |
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCaptureDefaultWhenCapturingThisCheck.cpp | ||
---|---|---|
82 | oh yes, I forgot to mark addressed. |
Thanks for updating the comment! Since I marked it as approved last time, I will land this in the next couple of days if no more comments come up.
Please merge these two lines. Amount of dashes and equal signs could be reduced.