carlosgalvezp requested changes to this revision.
carlosgalvezp added a comment.
This revision now requires changes to proceed.
Looking good! Some minor comments
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCapturingLambdaCoroutinesCheck.cpp:40
+
+ diag(Lambda->getBeginLoc(), "found capturing coroutine lambda");
+}
----------------
Perhaps it would be good with a diagnostic that gives more actionable feedback?
For example "avoid capturing coroutines in a lambda"
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCapturingLambdaCoroutinesCheck.h:18
+
+/// The normal usage of captures in lambdas are problematic when the lambda is
a
+/// coroutine because the captures are destroyed after the first suspension
----------------
Start the documentation describing "what" the check warns for (here you explain
the "why"). Make sure this text is in sync with the .rst documentation.
================
Comment at:
clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidCapturingLambdaCoroutinesCheck.h:28
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
----------------
Limit to C++20 using isLanguageVersionSupported
================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:121
+
+ Adds check for cpp core guideline: "CP.51: Do not use capturing lambdas that
+ are coroutines."
----------------
C++ Core Guidelines
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-capturing-lambda-coroutines.rst:23
+All of the cases above will trigger the warning. However, implicit captures
+do not trigger the warning unless the body of the lambda uses the capture.
+For example, the following do not trigger the warning.
----------------
Mention that the check implements rule CP.51 of C++ Core Guidelines, and add a
link to the that guideline (see other C++ Core Guidelines checks as an example).
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-avoid-capturing-lambda-coroutines.rst:24
+do not trigger the warning unless the body of the lambda uses the capture.
+For example, the following do not trigger the warning.
+
----------------
does
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:180
`concurrency-thread-canceltype-asynchronous
<concurrency/thread-canceltype-asynchronous.html>`_,
+ `cppcoreguidelines-avoid-capturing-lambda-coroutines
<cppcoreguidelines-avoid-capturing-lambda-coroutines.html>`_, "Yes"
`cppcoreguidelines-avoid-const-or-ref-data-members
<cppcoreguidelines/avoid-const-or-ref-data-members.html>`_,
----------------
Remove, since this check does not provide fix-its.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-capturing-lambda-coroutines.cpp:1-2
+// RUN: %check_clang_tidy -std=c++20-or-later %s
cppcoreguidelines-avoid-capturing-lambda-coroutines %t -- -- \
+// RUN: -isystem %S/readability/Inputs/identifier-naming/system
+
----------------
I think it's easier to read and reason about if you keep it in one line.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-avoid-capturing-lambda-coroutines.cpp:2
+// RUN: %check_clang_tidy -std=c++20-or-later %s
cppcoreguidelines-avoid-capturing-lambda-coroutines %t -- -- \
+// RUN: -isystem %S/readability/Inputs/identifier-naming/system
+
----------------
It's strange to have this test depend on input data from another module -
create a Inputs folder inside the cppcoreguidelines folder and copy
coroutines.h there.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137514/new/
https://reviews.llvm.org/D137514
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
