[PATCH] D137514: [clang-tidy] add check for capturing lambda coroutines

[Carlos Galvez via Phabricator via cfe-commits]

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