https://github.com/hjanuschka updated https://github.com/llvm/llvm-project/pull/182081
>From 8b411703bb89f0f603e7dd5f3d7a1b0628de7ca4 Mon Sep 17 00:00:00 2001 From: Helmut Januschka <[email protected]> Date: Wed, 18 Feb 2026 19:03:52 +0100 Subject: [PATCH] [clang-tidy] Add modernize-use-return-value check Add new clang-tidy check that finds functions returning void with a single non-const reference output parameter, suggesting they return the value directly instead. Returning values instead of using output parameters is generally preferred in modern C++ because it is clearer, works better with auto and structured bindings, and enables copy/move elision. For example: ```cpp // Before void getResult(int &Out) { Out = compute(); } // After int getResult() { return compute(); } ``` The check skips functions that: - return non-void - have zero or more than one non-const reference parameter - never assign to the output parameter - have abstract or array type output parameters - are virtual methods No fix-its are provided since changing the return type requires updating all call sites, which may span translation units. --- .../clang-tidy/modernize/CMakeLists.txt | 1 + .../modernize/ModernizeTidyModule.cpp | 3 + .../modernize/UseReturnValueCheck.cpp | 164 ++++++++++++++++++ .../modernize/UseReturnValueCheck.h | 34 ++++ clang-tools-extra/docs/ReleaseNotes.rst | 6 + .../docs/clang-tidy/checks/list.rst | 1 + .../checks/modernize/use-return-value.rst | 39 +++++ .../checkers/modernize/use-return-value.cpp | 73 ++++++++ 8 files changed, 321 insertions(+) create mode 100644 clang-tools-extra/clang-tidy/modernize/UseReturnValueCheck.cpp create mode 100644 clang-tools-extra/clang-tidy/modernize/UseReturnValueCheck.h create mode 100644 clang-tools-extra/docs/clang-tidy/checks/modernize/use-return-value.rst create mode 100644 clang-tools-extra/test/clang-tidy/checkers/modernize/use-return-value.cpp diff --git a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt index cc4cc7a02b594..312ba733d59ca 100644 --- a/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/modernize/CMakeLists.txt @@ -45,6 +45,7 @@ add_clang_library(clangTidyModernizeModule STATIC UseNullptrCheck.cpp UseOverrideCheck.cpp UseRangesCheck.cpp + UseReturnValueCheck.cpp UseScopedLockCheck.cpp UseStartsEndsWithCheck.cpp UseStdFormatCheck.cpp diff --git a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp index fcb860d8c5298..40ab9e593f6d5 100644 --- a/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/modernize/ModernizeTidyModule.cpp @@ -45,6 +45,7 @@ #include "UseNullptrCheck.h" #include "UseOverrideCheck.h" #include "UseRangesCheck.h" +#include "UseReturnValueCheck.h" #include "UseScopedLockCheck.h" #include "UseStartsEndsWithCheck.h" #include "UseStdFormatCheck.h" @@ -92,6 +93,8 @@ class ModernizeModule : public ClangTidyModule { CheckFactories.registerCheck<UseIntegerSignComparisonCheck>( "modernize-use-integer-sign-comparison"); CheckFactories.registerCheck<UseRangesCheck>("modernize-use-ranges"); + CheckFactories.registerCheck<UseReturnValueCheck>( + "modernize-use-return-value"); CheckFactories.registerCheck<UseScopedLockCheck>( "modernize-use-scoped-lock"); CheckFactories.registerCheck<UseStartsEndsWithCheck>( diff --git a/clang-tools-extra/clang-tidy/modernize/UseReturnValueCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseReturnValueCheck.cpp new file mode 100644 index 0000000000000..7568f7af989c6 --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/UseReturnValueCheck.cpp @@ -0,0 +1,164 @@ +//===----------------------------------------------------------------------===// +// +// 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 "UseReturnValueCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/RecursiveASTVisitor.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::modernize { + +namespace { + +/// Find the single non-const lvalue reference parameter in a function that +/// could serve as an output parameter. Returns nullptr if there are zero or +/// more than one such parameters, or if the parameter type is not suitable. +static const ParmVarDecl *findSingleOutParam(const FunctionDecl *Func) { + const ParmVarDecl *Candidate = nullptr; + for (const auto *Param : Func->parameters()) { + const QualType T = Param->getType(); + if (!T->isLValueReferenceType()) + continue; + const QualType Pointee = T->getPointeeType(); + // Skip const references -- those are input parameters. + if (Pointee.isConstQualified()) + continue; + // Skip references to non-object types. + if (Pointee->isFunctionType() || Pointee->isVoidType()) + continue; + // More than one non-const ref param -- ambiguous. + if (Candidate) + return nullptr; + Candidate = Param; + } + return Candidate; +} + +/// Visitor that checks whether a parameter is only written to (never read +/// before being fully assigned). A parameter is an "output-only" parameter if +/// every use of it in the function body is an assignment target or passed to +/// a function that takes a non-const reference (i.e., it is used purely for +/// output). +class OutParamVisitor : public RecursiveASTVisitor<OutParamVisitor> { +public: + explicit OutParamVisitor(const ParmVarDecl *Param) : Param(Param) {} + + bool isOutputOnly() const { return HasWrite && !HasRead; } + + bool VisitDeclRefExpr(const DeclRefExpr *DRE) { + if (DRE->getDecl() != Param) + return true; + + // Check how this reference is used by looking at the parent. + // We conservatively mark any use we cannot classify as a read. + HasWrite = true; + HasRead = true; // Conservative default; refined by parent checks. + return true; + } + + // Override to check parent context of DeclRefExprs. + bool TraverseStmt(Stmt *S) { + if (!S) + return true; + return RecursiveASTVisitor::TraverseStmt(S); + } + + /// Check if the param is used as an assignment LHS: + /// param = expr; (direct assignment) + /// param.field = expr; (member assignment) + bool VisitBinaryOperator(const BinaryOperator *BO) { + if (BO->getOpcode() != BO_Assign) + return true; + const Expr *LHS = BO->getLHS()->IgnoreImplicit(); + // Direct assignment: param = expr. + if (const auto *DRE = dyn_cast<DeclRefExpr>(LHS)) + if (DRE->getDecl() == Param) + FoundAssignment = true; + // Member assignment: param.field = expr. + if (const auto *ME = dyn_cast<MemberExpr>(LHS)) + if (const auto *DRE = + dyn_cast<DeclRefExpr>(ME->getBase()->IgnoreImplicit())) + if (DRE->getDecl() == Param) + FoundAssignment = true; + return true; + } + + bool hasAssignment() const { return FoundAssignment; } + +private: + const ParmVarDecl *Param; + bool HasWrite = false; + bool HasRead = false; + bool FoundAssignment = false; +}; + +} // namespace + +void UseReturnValueCheck::registerMatchers(MatchFinder *Finder) { + // Match non-template function definitions returning void. + Finder->addMatcher( + functionDecl(isDefinition(), unless(isImplicit()), unless(isDeleted()), + returns(voidType()), + unless(hasParent(functionTemplateDecl())), + // At least one non-const ref parameter. + hasAnyParameter(parmVarDecl(hasType(lValueReferenceType( + pointee(unless(isConstQualified()))))))) + .bind("func"), + this); +} + +void UseReturnValueCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Func = Result.Nodes.getNodeAs<FunctionDecl>("func"); + if (!Func || !Func->hasBody()) + return; + + // Skip system headers. + if (Result.SourceManager->isInSystemHeader(Func->getLocation())) + return; + + // Skip virtual methods (changing return type breaks polymorphism). + if (const auto *MD = dyn_cast<CXXMethodDecl>(Func)) + if (MD->isVirtual()) + return; + + // Skip main. + if (Func->isMain()) + return; + + // Find the single output parameter. + const ParmVarDecl *OutParam = findSingleOutParam(Func); + if (!OutParam || !OutParam->getIdentifier()) + return; + + // The output parameter type must be copyable/movable (not an abstract + // class, not an array, etc.). + const QualType ParamType = OutParam->getType()->getPointeeType(); + if (ParamType->isArrayType() || ParamType->isIncompleteType()) + return; + if (const auto *RD = ParamType->getAsCXXRecordDecl()) + if (RD->isAbstract()) + return; + + // Verify the parameter is actually assigned to in the body. + OutParamVisitor Visitor(OutParam); + Visitor.TraverseStmt(Func->getBody()); + + if (!Visitor.hasAssignment()) + return; + + diag(Func->getLocation(), + "function '%0' has output parameter '%1'; consider returning " + "the value directly instead") + << Func->getName() << OutParam->getName(); + diag(OutParam->getLocation(), "output parameter declared here", + DiagnosticIDs::Note); +} + +} // namespace clang::tidy::modernize diff --git a/clang-tools-extra/clang-tidy/modernize/UseReturnValueCheck.h b/clang-tools-extra/clang-tidy/modernize/UseReturnValueCheck.h new file mode 100644 index 0000000000000..4cd636c82445f --- /dev/null +++ b/clang-tools-extra/clang-tidy/modernize/UseReturnValueCheck.h @@ -0,0 +1,34 @@ +//===----------------------------------------------------------------------===// +// +// 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_USERETURNVALUECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USERETURNVALUECHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::modernize { + +/// Finds functions that return void and have a single non-const reference +/// output parameter, suggesting they return the value directly instead. +/// +/// For the user-facing documentation see: +/// https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-return-value.html +class UseReturnValueCheck : public ClangTidyCheck { +public: + UseReturnValueCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus; + } +}; + +} // namespace clang::tidy::modernize + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USERETURNVALUECHECK_H diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 68bab88146241..712d0dd18bb59 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -121,6 +121,12 @@ New checks ``llvm::to_vector(llvm::make_filter_range(...))`` that can be replaced with ``llvm::map_to_vector`` and ``llvm::filter_to_vector``. +- New :doc:`modernize-use-return-value + <clang-tidy/checks/modernize/use-return-value>` check. + + Finds ``void`` functions with a single non-const reference output + parameter, suggesting they return the value directly instead. + - New :doc:`modernize-use-string-view <clang-tidy/checks/modernize/use-string-view>` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst index c475870ed7b31..c52be3c225bff 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -325,6 +325,7 @@ Clang-Tidy Checks :doc:`modernize-use-nullptr <modernize/use-nullptr>`, "Yes" :doc:`modernize-use-override <modernize/use-override>`, "Yes" :doc:`modernize-use-ranges <modernize/use-ranges>`, "Yes" + :doc:`modernize-use-return-value <modernize/use-return-value>`, :doc:`modernize-use-scoped-lock <modernize/use-scoped-lock>`, "Yes" :doc:`modernize-use-starts-ends-with <modernize/use-starts-ends-with>`, "Yes" :doc:`modernize-use-std-format <modernize/use-std-format>`, "Yes" diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-return-value.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-return-value.rst new file mode 100644 index 0000000000000..281647f4d6f8c --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-return-value.rst @@ -0,0 +1,39 @@ +.. title:: clang-tidy - modernize-use-return-value + +modernize-use-return-value +========================== + +Finds functions that return ``void`` and have a single non-const +reference output parameter, suggesting they return the value +directly instead. + +Returning values instead of using output parameters is generally +preferred in modern C++ because it is clearer, works better with +``auto`` and structured bindings, and enables copy/move elision. + +.. code-block:: c++ + + // Before + void getResult(int &Out) { + Out = compute(); + } + + // After + int getResult() { + return compute(); + } + +The check will not flag a function if it: + +- returns non-void, +- has zero or more than one non-const reference parameter, +- never assigns to the output parameter, +- has an output parameter of abstract or array type, +- is a virtual method, or +- has an unnamed output parameter. + +.. note:: + + This check does not provide fix-its because changing the + return type requires updating all call sites, which may + span multiple translation units. diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-return-value.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-return-value.cpp new file mode 100644 index 0000000000000..c748606e7deab --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-return-value.cpp @@ -0,0 +1,73 @@ +// RUN: %check_clang_tidy %s modernize-use-return-value %t + +struct Widget { + int X; +}; + +// Positive: void function with single non-const ref output param. +void getInt(int &Out) { +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'getInt' has output parameter 'Out' + Out = 42; +} + +// Positive: struct output parameter. +void getWidget(Widget &Out) { +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'getWidget' has output parameter 'Out' + Out.X = 10; +} + +// Positive: mixed const and non-const ref params (one non-const). +void transform(const int &In, int &Out) { +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'transform' has output parameter 'Out' + Out = In * 2; +} + +// Positive: non-const ref + value params. +void compute(int A, int B, int &Result) { +// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: function 'compute' has output parameter 'Result' + Result = A + B; +} + +// Negative: returns non-void. +int getX(int &Out) { + Out = 42; + return 0; +} + +// Negative: const ref parameter (input, not output). +void readOnly(const int &X) { +} + +// Negative: multiple non-const ref params (ambiguous outparam). +void swap(int &A, int &B) { + int T = A; + A = B; + B = T; +} + +// Negative: no assignment to the parameter. +void noWrite(int &X) { + int Y = X + 1; +} + +// Negative: virtual method. +struct Base { + virtual void vmethod(int &Out); +}; + +// Negative: abstract output type. +struct Abstract { + virtual void foo() = 0; +}; +void getAbstract(Abstract &Out) { + // Not flagged -- Abstract cannot be returned by value. +} + +// Negative: array type. +void getArray(int (&Out)[10]) { + Out[0] = 1; +} + +// Negative: unnamed parameter. +void unnamed(int &) { +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
