-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[clang-tidy] Add modernize-make-direct check #118120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ With the latest revision this PR passed the C/C++ code formatter. |
This will conflict with I also very sure having Having no insight on the differences of the inner workings - but just based on https://en.cppreference.com/w/cpp/memory/shared_ptr/make_shared#Notes this appears to be something behaving very differently and less optimal/safe. |
clang-tools-extra/clang-tidy/modernize/MakeFunctionToDirectCheck.h
Outdated
Show resolved
Hide resolved
clang-tools-extra/docs/clang-tidy/checks/modernize/modernize-make-direct.rst
Outdated
Show resolved
Hide resolved
clang-tools-extra/clang-tidy/modernize/MakeFunctionToDirectCheck.cpp
Outdated
Show resolved
Hide resolved
Adds a new check that converts std::make_* function calls to direct constructor calls using CTAD. Transforms make_optional, make_unique, make_shared and make_pair into their equivalent direct constructor calls, leveraging C++17's class template argument deduction.
- Refactor implementation to use TransformerClangTidyCheck for cleaner code - Remove make_unique and make_shared transformations to avoid conflicts and new usage - Add configuration options: CheckMakePair, CheckMakeOptional, CheckMakeTuple - Add support for std::make_tuple transformations - Update documentation with options and warnings about smart pointer functions - Fix clang-format issues in ModernizeTidyModule.cpp - Add C++17 requirement enforcement with proper test setup - All tests passing with correct CTAD transformations
- Apply clang-format fixes to MakeFunctionToDirectCheck.cpp - Fix RST title underline length in modernize-make-direct.rst documentation - All tests passing after fixes
- Rename modernize-make-direct.rst to make-direct.rst to follow convention - Fix ReleaseNotes.rst document reference path - This resolves 'unknown document' Sphinx errors in CI
@denzor200 feedback addressed. thank you both for your time, i kind of missed notifications for this PR |
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Helmut Januschka (hjanuschka) ChangesAdds a new check that converts std::make_* function calls to direct constructor calls using CTAD. Transforms make_optional, make_unique, make_shared and make_pair into their equivalent direct constructor calls, leveraging C++17's class template argument deduction. Full diff: https://github.com/llvm/llvm-project/pull/118120.diff 7 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
index bab1167fb15ff..56273f914178b 100644
--- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt
@@ -50,6 +50,7 @@ add_clang_library(clangTidyModernizeModule STATIC
UseTransparentFunctorsCheck.cpp
UseUncaughtExceptionsCheck.cpp
UseUsingCheck.cpp
+ MakeFunctionToDirectCheck.cpp
LINK_LIBS
clangTidy
diff --git a/clang-tools-extra/clang-tidy/modernize/MakeFunctionToDirectCheck.cpp b/clang-tools-extra/clang-tidy/modernize/MakeFunctionToDirectCheck.cpp
new file mode 100644
index 0000000000000..f9eabbaefb46f
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/modernize/MakeFunctionToDirectCheck.cpp
@@ -0,0 +1,78 @@
+//===--- MakeFunctionToDirectCheck.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 "MakeFunctionToDirectCheck.h"
+#include "../utils/TransformerClangTidyCheck.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Transformer/RangeSelector.h"
+#include "clang/Tooling/Transformer/RewriteRule.h"
+#include "clang/Tooling/Transformer/Stencil.h"
+
+using namespace ::clang::ast_matchers;
+using namespace ::clang::transformer;
+
+namespace clang::tidy::modernize {
+
+namespace {
+
+RewriteRuleWith<std::string>
+makeFunctionToDirectCheckImpl(bool CheckMakePair, bool CheckMakeOptional,
+ bool CheckMakeTuple) {
+ std::vector<RewriteRuleWith<std::string>> Rules;
+
+ // Helper to create a rule for a specific make_* function
+ auto createRule = [](StringRef MakeFunction, StringRef DirectType) {
+ auto WarningMessage = cat("use class template argument deduction (CTAD) "
+ "instead of ",
+ MakeFunction);
+
+ return makeRule(
+ callExpr(callee(functionDecl(hasName(MakeFunction))),
+ unless(hasParent(implicitCastExpr(hasImplicitDestinationType(
+ qualType(hasCanonicalType(qualType(asString("void")))))))))
+ .bind("make_call"),
+ changeTo(node("make_call"),
+ cat(DirectType, "(", callArgs("make_call"), ")")),
+ WarningMessage);
+ };
+
+ if (CheckMakeOptional) {
+ Rules.push_back(createRule("std::make_optional", "std::optional"));
+ }
+
+ if (CheckMakePair) {
+ Rules.push_back(createRule("std::make_pair", "std::pair"));
+ }
+
+ if (CheckMakeTuple) {
+ Rules.push_back(createRule("std::make_tuple", "std::tuple"));
+ }
+
+ return applyFirst(Rules);
+}
+
+} // namespace
+
+MakeFunctionToDirectCheck::MakeFunctionToDirectCheck(StringRef Name,
+ ClangTidyContext *Context)
+ : utils::TransformerClangTidyCheck(Name, Context),
+ CheckMakePair(Options.get("CheckMakePair", true)),
+ CheckMakeOptional(Options.get("CheckMakeOptional", true)),
+ CheckMakeTuple(Options.get("CheckMakeTuple", true)) {
+ setRule(makeFunctionToDirectCheckImpl(CheckMakePair, CheckMakeOptional,
+ CheckMakeTuple));
+}
+
+void MakeFunctionToDirectCheck::storeOptions(
+ ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "CheckMakePair", CheckMakePair);
+ Options.store(Opts, "CheckMakeOptional", CheckMakeOptional);
+ Options.store(Opts, "CheckMakeTuple", CheckMakeTuple);
+}
+
+} // namespace clang::tidy::modernize
\ No newline at end of file
diff --git a/clang-tools-extra/clang-tidy/modernize/MakeFunctionToDirectCheck.h b/clang-tools-extra/clang-tidy/modernize/MakeFunctionToDirectCheck.h
new file mode 100644
index 0000000000000..c3637b2bc00dc
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/modernize/MakeFunctionToDirectCheck.h
@@ -0,0 +1,39 @@
+//===--- MakeFunctionToDirectCheck.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_MODERNIZE_MAKEFUNCTIONTODIRECTCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_MAKEFUNCTIONTODIRECTCHECK_H
+
+#include "../utils/TransformerClangTidyCheck.h"
+
+namespace clang::tidy::modernize {
+
+/// Converts std::make_* function calls to direct constructor calls using
+/// class template argument deduction (CTAD).
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize/make-direct.html
+class MakeFunctionToDirectCheck : public utils::TransformerClangTidyCheck {
+public:
+ MakeFunctionToDirectCheck(StringRef Name, ClangTidyContext *Context);
+
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+ return LangOpts.CPlusPlus17;
+ }
+
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+ const bool CheckMakePair;
+ const bool CheckMakeOptional;
+ const bool CheckMakeTuple;
+};
+
+} // namespace clang::tidy::modernize
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_MAKEFUNCTIONTODIRECTCHECK_H
\ No newline at end of file
diff --git a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
index e872759856f3c..4c6964a729346 100644
--- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp
@@ -16,6 +16,7 @@
#include "DeprecatedIosBaseAliasesCheck.h"
#include "LoopConvertCheck.h"
#include "MacroToEnumCheck.h"
+#include "MakeFunctionToDirectCheck.h"
#include "MakeSharedCheck.h"
#include "MakeUniqueCheck.h"
#include "MinMaxUseInitializerListCheck.h"
@@ -125,6 +126,8 @@ class ModernizeModule : public ClangTidyModule {
CheckFactories.registerCheck<UseUncaughtExceptionsCheck>(
"modernize-use-uncaught-exceptions");
CheckFactories.registerCheck<UseUsingCheck>("modernize-use-using");
+ CheckFactories.registerCheck<MakeFunctionToDirectCheck>(
+ "modernize-make-direct");
}
};
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 882ee0015df17..8917627b13ea0 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -124,6 +124,12 @@ New checks
pointer and store it as class members without handle the copy and move
constructors and the assignments.
+- New :doc:`modernize-make-direct <clang-tidy/checks/modernize/make-direct>` check.
+
+ Converts std::make_* function calls to direct constructor calls using CTAD.
+ Transforms make_optional, make_pair, and make_tuple into equivalent
+ direct constructor calls using C++17's class template argument deduction.
+
- New :doc:`bugprone-misleading-setter-of-reference
<clang-tidy/checks/bugprone/misleading-setter-of-reference>` check.
diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/make-direct.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/make-direct.rst
new file mode 100644
index 0000000000000..b856c150f28cf
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/make-direct.rst
@@ -0,0 +1,46 @@
+.. title:: clang-tidy - modernize-make-direct
+
+modernize-make-direct
+=====================
+
+Replaces ``std::make_*`` function calls with direct constructor calls using class template
+argument deduction (CTAD).
+
+================================== ====================================
+ Before After
+---------------------------------- ------------------------------------
+``std::make_optional<int>(42)`` ``std::optional(42)``
+``std::make_pair(1, "test")`` ``std::pair(1, "test")``
+``std::make_tuple(1, 2.0, "hi")`` ``std::tuple(1, 2.0, "hi")``
+================================== ====================================
+
+.. note::
+
+ This check does not transform ``std::make_unique`` or ``std::make_shared`` because:
+
+ 1. These smart pointer types cannot be constructed using CTAD from raw pointers.
+ 2. ``std::make_shared`` provides performance benefits (single allocation) and
+ exception safety that would be lost with direct construction.
+ 3. Direct use of ``new`` is discouraged in modern C++ code.
+
+ Use the dedicated ``modernize-make-unique`` and ``modernize-make-shared`` checks
+ for transforming these functions.
+
+Options
+-------
+
+.. option:: CheckMakePair
+
+ When `true`, transforms ``std::make_pair`` calls to direct constructor calls.
+ Default is `true`.
+
+.. option:: CheckMakeOptional
+
+ When `true`, transforms ``std::make_optional`` calls to direct constructor calls.
+ Default is `true`.
+
+.. option:: CheckMakeTuple
+
+ When `true`, transforms ``std::make_tuple`` calls to direct constructor calls.
+ Default is `true`.
+
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/make-direct-check.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/make-direct-check.cpp
new file mode 100644
index 0000000000000..2a1fe4713ecb9
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/make-direct-check.cpp
@@ -0,0 +1,76 @@
+// RUN: %check_clang_tidy -std=c++17 %s modernize-make-direct %t
+
+namespace std {
+template<typename T>
+struct optional {
+ optional(const T&) {}
+};
+
+template<typename T>
+optional<T> make_optional(const T& t) { return optional<T>(t); }
+
+template<typename T>
+struct unique_ptr {
+ explicit unique_ptr(T*) {}
+};
+
+template<typename T, typename... Args>
+unique_ptr<T> make_unique(Args&&... args) {
+ return unique_ptr<T>(new T(args...));
+}
+
+template<typename T>
+struct shared_ptr {
+ shared_ptr(T*) {}
+};
+
+template<typename T, typename... Args>
+shared_ptr<T> make_shared(Args&&... args) {
+ return shared_ptr<T>(new T(args...));
+}
+
+template<typename T, typename U>
+struct pair {
+ T first;
+ U second;
+ pair(const T& x, const U& y) : first(x), second(y) {}
+};
+
+template<typename T, typename U>
+pair<T,U> make_pair(T&& t, U&& u) {
+ return pair<T,U>(t, u);
+}
+
+template<typename... T>
+struct tuple {
+ tuple(const T&... args) {}
+};
+
+template<typename... T>
+tuple<T...> make_tuple(T&&... t) {
+ return tuple<T...>(t...);
+}
+}
+
+struct Widget {
+ Widget(int x) {}
+};
+
+
+void basic_tests() {
+ auto o1 = std::make_optional<int>(42);
+ // CHECK-MESSAGES: warning: use class template argument deduction (CTAD) instead of std::make_optional [modernize-make-direct]
+ // CHECK-FIXES: auto o1 = std::optional(42);
+
+ // make_unique and make_shared are not transformed by this check
+ auto u1 = std::make_unique<Widget>(1);
+ auto s1 = std::make_shared<Widget>(2);
+
+ auto p1 = std::make_pair(1, "test");
+ // CHECK-MESSAGES: warning: use class template argument deduction (CTAD) instead of std::make_pair [modernize-make-direct]
+ // CHECK-FIXES: auto p1 = std::pair(1, "test");
+
+ auto t1 = std::make_tuple(1, 2.0, "hi");
+ // CHECK-MESSAGES: warning: use class template argument deduction (CTAD) instead of std::make_tuple [modernize-make-direct]
+ // CHECK-FIXES: auto t1 = std::tuple(1, 2.0, "hi");
+}
|
|
||
} // namespace clang::tidy::modernize | ||
|
||
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_MAKEFUNCTIONTODIRECTCHECK_H No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add newline.
|
||
- New :doc:`modernize-make-direct <clang-tidy/checks/modernize/make-direct>` check. | ||
|
||
Converts std::make_* function calls to direct constructor calls using CTAD. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converts std::make_* function calls to direct constructor calls using CTAD. | |
Converts ``std::make_*`` function calls to direct constructor calls using CTAD. |
- New :doc:`modernize-make-direct <clang-tidy/checks/modernize/make-direct>` check. | ||
|
||
Converts std::make_* function calls to direct constructor calls using CTAD. | ||
Transforms make_optional, make_pair, and make_tuple into equivalent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single sentence is enough.
modernize-make-direct | ||
===================== | ||
|
||
Replaces ``std::make_*`` function calls with direct constructor calls using class template |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please synchronize with statement in Release Notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally good check idea, but needs some polishing.
- Neither
modernize-make-function-to-direct
normodernize-make-direct
sounds clear to me (specificallydirect
part), the main purpose of this transformation is to useCTAD
, it should be in the check name at least.modernize-use-ctad-constructor
could be it, but I'm not 100% sure others will agree. - I don't like having many options that all do the same job. How about a list of key-value pairs of classes and their corresponding
make_
function. Something like "std::pair=std::make_pair;std::optional=std::make_optional...". Look atreadability-suspicious-call-argument.Abbreviations
for reference. - Enhance tests with type aliases, macros, template parameters
UseTransparentFunctorsCheck.cpp | ||
UseUncaughtExceptionsCheck.cpp | ||
UseUsingCheck.cpp | ||
MakeFunctionToDirectCheck.cpp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please format alphabetically
Options.store(Opts, "CheckMakeTuple", CheckMakeTuple); | ||
} | ||
|
||
} // namespace clang::tidy::modernize No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add newline
|
||
} // namespace clang::tidy::modernize | ||
|
||
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_MAKEFUNCTIONTODIRECTCHECK_H No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add newline
"modernize-use-uncaught-exceptions"); | ||
CheckFactories.registerCheck<UseUsingCheck>("modernize-use-using"); | ||
CheckFactories.registerCheck<MakeFunctionToDirectCheck>( | ||
"modernize-make-direct"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please format alphabetically
pointer and store it as class members without handle the copy and move | ||
constructors and the assignments. | ||
|
||
- New :doc:`modernize-make-direct <clang-tidy/checks/modernize/make-direct>` check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please format alphabetically and place doc string same as other checks. <>
should be on the next line
|
||
- New :doc:`modernize-make-direct <clang-tidy/checks/modernize/make-direct>` check. | ||
|
||
Converts std::make_* function calls to direct constructor calls using CTAD. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please sync with docs first sentence (should be identical)
@@ -0,0 +1,46 @@ | |||
.. title:: clang-tidy - modernize-make-direct | |||
|
|||
modernize-make-direct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sync check name accross all files, somewhere you have "MakeFunctionToDirect".
unless(hasParent(implicitCastExpr(hasImplicitDestinationType( | ||
qualType(hasCanonicalType(qualType(asString("void"))))))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please elaborate why do you need this unless(hasParen(...))
in your check?
I didn't find tests that will use this behavior, or I couldn't understand what existing test used it:(
RewriteRuleWith<std::string> | ||
makeFunctionToDirectCheckImpl(bool CheckMakePair, bool CheckMakeOptional, | ||
bool CheckMakeTuple) { | ||
std::vector<RewriteRuleWith<std::string>> Rules; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider using llvm::SmallVector
// CHECK-MESSAGES: warning: use class template argument deduction (CTAD) instead of std::make_pair [modernize-make-direct] | ||
// CHECK-FIXES: auto p1 = std::pair(1, "test"); | ||
|
||
auto t1 = std::make_tuple(1, 2.0, "hi"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add
const int arg = 2.0;
auto t2 = std::make_tuple(1, std::cref(arg), "hi");
this check must be silent in this case
This comment was marked as resolved.
This comment was marked as resolved.
Options | ||
------- | ||
|
||
.. option:: CheckMakePair |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should instead be 1 option with a semicolon separated list of which functions to transform.
exception safety that would be lost with direct construction. | ||
3. Direct use of ``new`` is discouraged in modern C++ code. | ||
|
||
Use the dedicated ``modernize-make-unique`` and ``modernize-make-shared`` checks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing, since these checks do the exact opposite of what this check is doing. I would vote for removing this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of minor changes.
I'm not convinced about the name of the check, make-function-to-direct
does not really say much. What is a "make function" and what is "direct"?
Would it make sense to call it modernize-use-ctad
and possibly leave the door open for more use cases in follow-up patches?
PS: please use the rename_check.py
script to rename the check, to ensure all places get updated. But let's wait until a consensus is reached before churning the code with name changes.
+1 on |
@hjanuschka, do you plan on finishing this check? If not, I'm happy to pick it up and finish it; I think it's a good check and I want to see it land. |
@localspook damn, i missed that one, kind of like forgot about it! |
Alright, I'll take it on! |
Fixes #121295.
Adds a new check that converts std::make_* function calls to direct constructor calls using CTAD. Transforms make_optional, make_unique, make_shared and make_pair into their equivalent direct constructor calls, leveraging C++17's class template argument deduction.