From 16b07c866ae74e0d02038574fe1c37a6cb55e233 Mon Sep 17 00:00:00 2001 From: CJ Johnson Date: Mon, 8 Nov 2021 15:40:18 +0000 Subject: [PATCH] [clang-tidy] Add check for initialization of `absl::Cleanup`. Suggests switching the initialization pattern of `absl::Cleanup` instances from the factory function to class template argument deduction (CTAD) in C++17 and higher. Reviewed By: ymandel Differential Revision: https://reviews.llvm.org/D113195 --- .../clang-tidy/abseil/AbseilTidyModule.cpp | 2 + .../clang-tidy/abseil/CMakeLists.txt | 1 + .../clang-tidy/abseil/CleanupCtadCheck.cpp | 49 ++++++++ .../clang-tidy/abseil/CleanupCtadCheck.h | 37 ++++++ clang-tools-extra/docs/ReleaseNotes.rst | 7 ++ .../clang-tidy/checks/abseil-cleanup-ctad.rst | 22 ++++ .../docs/clang-tidy/checks/list.rst | 2 + .../checkers/abseil-cleanup-ctad.cpp | 115 ++++++++++++++++++ 8 files changed, 235 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/abseil-cleanup-ctad.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/abseil-cleanup-ctad.cpp diff --git a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp b/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp index 7d592d7e3e55..5d99e6b50075 100644 --- a/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/abseil/AbseilTidyModule.cpp @@ -9,6 +9,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "CleanupCtadCheck.h" #include "DurationAdditionCheck.h" #include "DurationComparisonCheck.h" #include "DurationConversionCastCheck.h" @@ -35,6 +36,7 @@ namespace abseil { class AbseilModule : public ClangTidyModule { public: void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck("abseil-cleanup-ctad"); CheckFactories.registerCheck( "abseil-duration-addition"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt index b6ea21879daf..e7c86fc8107d 100644 --- a/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/abseil/CMakeLists.txt @@ -5,6 +5,7 @@ set(LLVM_LINK_COMPONENTS add_clang_library(clangTidyAbseilModule AbseilTidyModule.cpp + CleanupCtadCheck.cpp DurationAdditionCheck.cpp DurationComparisonCheck.cpp DurationConversionCastCheck.cpp diff --git a/clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.cpp b/clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.cpp new file mode 100644 index 000000000000..bc152f1dafa7 --- /dev/null +++ b/clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.cpp @@ -0,0 +1,49 @@ +//===--- CleanupCtadCheck.cpp - clang-tidy --------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "CleanupCtadCheck.h" +#include "../utils/TransformerClangTidyCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Tooling/Transformer/RangeSelector.h" +#include "clang/Tooling/Transformer/RewriteRule.h" +#include "clang/Tooling/Transformer/Stencil.h" +#include "llvm/ADT/StringRef.h" + +using namespace ::clang::ast_matchers; +using namespace ::clang::transformer; + +namespace clang { +namespace tidy { +namespace abseil { + +RewriteRule CleanupCtadCheckImpl() { + auto warning_message = cat("prefer absl::Cleanup's class template argument " + "deduction pattern in C++17 and higher"); + + return makeRule( + declStmt(has(varDecl( + hasType(autoType()), hasTypeLoc(typeLoc().bind("auto_type_loc")), + hasInitializer(traverse( + clang::TK_IgnoreUnlessSpelledInSource, + callExpr(callee(functionDecl(hasName("absl::MakeCleanup"))), + argumentCountIs(1), + hasArgument(0, expr().bind("make_cleanup_argument"))) + .bind("make_cleanup_call")))))), + {changeTo(node("auto_type_loc"), cat("absl::Cleanup")), + changeTo(node("make_cleanup_call"), cat(node("make_cleanup_argument")))}, + warning_message); +} + +CleanupCtadCheck::CleanupCtadCheck(StringRef Name, ClangTidyContext *Context) + : utils::TransformerClangTidyCheck(CleanupCtadCheckImpl(), Name, Context) {} + +} // namespace abseil +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.h b/clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.h new file mode 100644 index 000000000000..ce4e5c6be9d8 --- /dev/null +++ b/clang-tools-extra/clang-tidy/abseil/CleanupCtadCheck.h @@ -0,0 +1,37 @@ +//===--- CleanupCtadCheck.h - clang-tidy ------------------------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_CLEANUPCTADCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_CLEANUPCTADCHECK_H + +#include "../utils/TransformerClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace abseil { + +/// Suggests switching the initialization pattern of `absl::Cleanup` +/// instances from the factory function to class template argument +/// deduction (CTAD), in C++17 and higher. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-cleanup-ctad.html +class CleanupCtadCheck : public utils::TransformerClangTidyCheck { +public: + CleanupCtadCheck(StringRef Name, ClangTidyContext *Context); + + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus17; + } +}; + +} // namespace abseil +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_CLEANUPCTADCHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 5cd131e18ac8..41934d8fb905 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -76,6 +76,13 @@ Improvements to clang-tidy New checks ^^^^^^^^^^ +- New :doc:`abseil-cleanup-ctad + ` check. + + Suggests switching the initialization pattern of ``absl::Cleanup`` + instances from the factory function to class template argument + deduction (CTAD), in C++17 and higher. + - New :doc:`bugprone-suspicious-memory-comparison ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/abseil-cleanup-ctad.rst b/clang-tools-extra/docs/clang-tidy/checks/abseil-cleanup-ctad.rst new file mode 100644 index 000000000000..b8afc8b8a848 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/abseil-cleanup-ctad.rst @@ -0,0 +1,22 @@ +.. title:: clang-tidy - abseil-cleanup-ctad + +abseil-cleanup-ctad +=================== + +Suggests switching the initialization pattern of ``absl::Cleanup`` +instances from the factory function to class template argument +deduction (CTAD), in C++17 and higher. + +.. code-block:: c++ + + auto c1 = absl::MakeCleanup([] {}); + + const auto c2 = absl::MakeCleanup(std::function([] {})); + +becomes + +.. code-block:: c++ + + absl::Cleanup c1 = [] {}; + + const absl::Cleanup c2 = std::function([] {}); diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index edb656bc1e48..c9b4daf42ef0 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -12,6 +12,7 @@ Clang-Tidy Checks .. csv-table:: :header: "Name", "Offers fixes" + `abseil-cleanup-ctad `_, "Yes" `abseil-duration-addition `_, "Yes" `abseil-duration-comparison `_, "Yes" `abseil-duration-conversion-cast `_, "Yes" @@ -448,3 +449,4 @@ Clang-Tidy Checks `hicpp-vararg `_, `cppcoreguidelines-pro-type-vararg `_, `llvm-else-after-return `_, `readability-else-after-return `_, "Yes" `llvm-qualified-auto `_, `readability-qualified-auto `_, "Yes" + \ No newline at end of file diff --git a/clang-tools-extra/test/clang-tidy/checkers/abseil-cleanup-ctad.cpp b/clang-tools-extra/test/clang-tidy/checkers/abseil-cleanup-ctad.cpp new file mode 100644 index 000000000000..c023521bb261 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/abseil-cleanup-ctad.cpp @@ -0,0 +1,115 @@ +// RUN: %check_clang_tidy %s abseil-cleanup-ctad -std=c++17 %t + +namespace std { + +template +struct is_same { + static const bool value = false; +}; + +template +struct is_same { static const bool value = true; }; + +template +class function { +public: + template + function(T) {} + function(const function &) {} +}; + +} // namespace std + +namespace absl { + +namespace cleanup_internal { + +struct Tag {}; + +template +class Storage { +public: + Storage() = delete; + + explicit Storage(Callback callback) {} + + Storage(Storage &&other) {} + + Storage(const Storage &other) = delete; + + Storage &operator=(Storage &&other) = delete; + + Storage &operator=(const Storage &other) = delete; + +private: + bool is_callback_engaged_; + alignas(Callback) char callback_buffer_[sizeof(Callback)]; +}; + +} // namespace cleanup_internal + +template +class Cleanup final { +public: + Cleanup(Callback callback) // NOLINT + : storage_(static_cast(callback)) {} + + Cleanup(Cleanup &&other) = default; + + void Cancel() &&; + + void Invoke() &&; + + ~Cleanup(); + +private: + cleanup_internal::Storage storage_; +}; + +template +Cleanup(Callback callback) -> Cleanup; + +template +absl::Cleanup MakeCleanup(Callback callback) { + return {static_cast(callback)}; +} + +} // namespace absl + +void test() { + auto a = 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 a = [] {};{{$}} + + // Removes extra parens + auto b = absl::MakeCleanup(([] {})); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer absl::Cleanup{{.*}}C++17 and higher + // CHECK-FIXES: {{^}} absl::Cleanup b = [] {};{{$}} + + auto c = absl::MakeCleanup(std::function([] {})); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer absl::Cleanup{{.*}}C++17 and higher + // CHECK-FIXES: {{^}} absl::Cleanup c = std::function([] {});{{$}} + + // Removes extra parens + auto d = absl::MakeCleanup((std::function([] {}))); + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: prefer absl::Cleanup{{.*}}C++17 and higher + // CHECK-FIXES: {{^}} absl::Cleanup d = std::function([] {});{{$}} + + const auto e = absl::MakeCleanup([] {}); + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: prefer absl::Cleanup{{.*}}C++17 and higher + // CHECK-FIXES: {{^}} const absl::Cleanup e = [] {};{{$}} + + // Removes extra parens + const auto f = absl::MakeCleanup(([] {})); + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: prefer absl::Cleanup{{.*}}C++17 and higher + // CHECK-FIXES: {{^}} const absl::Cleanup f = [] {};{{$}} + + const auto g = absl::MakeCleanup(std::function([] {})); + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: prefer absl::Cleanup{{.*}}C++17 and higher + // CHECK-FIXES: {{^}} const absl::Cleanup g = std::function([] {});{{$}} + + // Removes extra parens + const auto h = absl::MakeCleanup((std::function([] {}))); + // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: prefer absl::Cleanup{{.*}}C++17 and higher + // CHECK-FIXES: {{^}} const absl::Cleanup h = std::function([] {});{{$}} +}