[PATCH] D113195: [clang-tidy] Add check for initialization of `absl::Cleanup`.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel accepted this revision.
ymandel added a comment.
This revision is now accepted and ready to land.

Nice!



================
Comment at: clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.cpp:19
+
+using namespace clang::ast_matchers;
+using namespace ::clang::transformer;
----------------



================
Comment at: clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.cpp:46-49
+bool CleanupCtadCheck::isLanguageVersionSupported(
+    const LangOptions &LangOpts) const {
+  return LangOpts.CPlusPlus17;
+}
----------------
nit: move this inline into the class declaration. Serves as documentation for the class then as well (in that a reader can look in the header and see the target language).


================
Comment at: clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.h:20
+/// instances from the factory function to class template argument
+/// deduction (CTAD) in C++17 and higher.
+///
----------------



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/abseil-cleanup-ctad.cpp:7-20
+template <typename, typename T>
+class Cleanup {
+public:
+  Cleanup(T) {}
+  Cleanup(Cleanup &&) {}
+};
+
----------------
Since the tests depend on the mock of the `Cleanup` declaration, have you run this on real code examples to double-check that it works as expected? Sometimes the divergence means real code can have unexpected implicit nodes that subtly impact the results, so it's always good to verify the mocks themselves in some way.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/abseil-cleanup-ctad.cpp:41
+
+  auto b = absl::MakeCleanup(([] {}));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer absl::Cleanup's class template argument deduction pattern in C++17 and higher
----------------
nit: this subtle, i'd add a comment pointing out the parens.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/abseil-cleanup-ctad.cpp:42
+  auto b = absl::MakeCleanup(([] {}));
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer absl::Cleanup's class template argument deduction pattern in C++17 and higher
+  // CHECK-FIXES: {{^}}  absl::Cleanup b = [] {};{{$}}
----------------
Here and below: you can truncate to 80 chars, any repeated message. We typically only check the whole message for the first appearance.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113195/new/

https://reviews.llvm.org/D113195