diff --git a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/readability/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/readability/CMakeLists.txt @@ -44,6 +44,7 @@ UniqueptrDeleteReleaseCheck.cpp UppercaseLiteralSuffixCheck.cpp UseAnyOfAllOfCheck.cpp + UselessReturnValueCheck.cpp LINK_LIBS clangTidy diff --git a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp --- a/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp @@ -47,6 +47,7 @@ #include "UniqueptrDeleteReleaseCheck.h" #include "UppercaseLiteralSuffixCheck.h" #include "UseAnyOfAllOfCheck.h" +#include "UselessReturnValueCheck.h" namespace clang { namespace tidy { @@ -109,6 +110,8 @@ "readability-static-definition-in-anonymous-namespace"); CheckFactories.registerCheck( "readability-string-compare"); + CheckFactories.registerCheck( + "readability-useless-return-value"); CheckFactories.registerCheck( "readability-named-parameter"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.h b/clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.h @@ -0,0 +1,38 @@ +//===--- UselessReturnValueCheck.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_READABILITY_USELESSRETURNVALUECHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USELESSRETURNVALUECHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang { +namespace tidy { +namespace readability { + +/// Looks for function that only and always return 0 and +/// proposes to make them void. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/readability-useless-return-value.html +class UselessReturnValueCheck : public ClangTidyCheck { +public: + UselessReturnValueCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + void replaceWithVoid(const FunctionDecl *MatchedDecl); +}; + +} // namespace readability +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_USELESSRETURNVALUECHECK_H diff --git a/clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.cpp b/clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.cpp @@ -0,0 +1,82 @@ +//===--- UselessReturnValueCheck.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 "UselessReturnValueCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang { +namespace tidy { +namespace readability { + +void UselessReturnValueCheck::registerMatchers(MatchFinder *Finder) { + auto Int0 = integerLiteral(equals(0)); + auto IgnInt0 = ignoringParenCasts(anyOf(Int0, parenExpr(has(Int0)))); + auto Int0Var = + anyOf(IgnInt0, + ignoringImplicit(declRefExpr( + to(varDecl(hasInitializer(IgnInt0), unless(hasGlobalStorage())) + .bind("retvar"))))); + Finder->addMatcher( + functionDecl( + isDefinition(), unless(returns(voidType())), unless(hasName("main")), + unless(cxxMethodDecl(isVirtual())), + forEachDescendant( + returnStmt(hasReturnValue(Int0Var)).bind("return-to-void")), + unless(hasDescendant(returnStmt(unless(hasReturnValue(Int0Var))))), + unless(hasDescendant(binaryOperator( + isAssignmentOperator(), + hasLHS(declRefExpr(to(varDecl(equalsBoundNode("retvar")))))))), + unless(hasDescendant(cxxOperatorCallExpr( + isAssignmentOperator(), + hasLHS(hasDescendant( + declRefExpr(to(varDecl(equalsBoundNode("retvar"))))))))), + unless(hasDescendant( + unaryOperator(hasAnyOperatorName("++", "--", "&"), + hasUnaryOperand(ignoringImplicit(declRefExpr( + to(varDecl(equalsBoundNode("retvar")))))))))) + .bind("function-to-void"), + this); +} + +void UselessReturnValueCheck::replaceWithVoid(const FunctionDecl *MatchedDecl) { + diag(MatchedDecl->getLocation(), "function %0 returns always the same value") + << MatchedDecl; + + diag(MatchedDecl->getLocation(), "make function void", DiagnosticIDs::Note) + << FixItHint::CreateReplacement(MatchedDecl->getReturnTypeSourceRange(), + "void"); +} + +void UselessReturnValueCheck::check(const MatchFinder::MatchResult &Result) { + const auto *MatchedDecl = + Result.Nodes.getNodeAs("function-to-void"); + if (!MatchedDecl->getIdentifier()) + return; + replaceWithVoid(MatchedDecl); + + const FunctionDecl *ProtoDecl = MatchedDecl->getCanonicalDecl(); + if (ProtoDecl) + replaceWithVoid(ProtoDecl); + + const auto *MatchedReturn = + Result.Nodes.getNodeAs("return-to-void"); + diag(MatchedReturn->getBeginLoc(), "returns 0 here"); + SourceLocation RemovalStartLocation = + MatchedReturn->getBeginLoc().getLocWithOffset(6); + SourceRange RemovalRange = + SourceRange(RemovalStartLocation, MatchedReturn->getEndLoc()); + diag(RemovalStartLocation, "remove return value", DiagnosticIDs::Note) + << FixItHint::CreateRemoval(RemovalRange); +} + +} // namespace readability +} // namespace tidy +} // namespace clang diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -69,6 +69,11 @@ The improvements are... + - New :doc:`readability-useless-return-value ` check. + + Finds functions that always return `0`, such functions could be ``void``. + + Improvements to include-fixer ----------------------------- diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -312,6 +312,7 @@ `readability-uniqueptr-delete-release `_, "Yes" `readability-uppercase-literal-suffix `_, "Yes" `readability-use-anyofallof `_, + `readability-useless-return-value `_, "Yes" `zircon-temporary-objects `_, diff --git a/clang-tools-extra/docs/clang-tidy/checks/readability-useless-return-value.rst b/clang-tools-extra/docs/clang-tidy/checks/readability-useless-return-value.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/readability-useless-return-value.rst @@ -0,0 +1,57 @@ +.. title:: clang-tidy - readability-useless-return-value + +readability-useless-return-value +================================ + +This check looks for functions that always return `0`. +Such functions could be ``void``. + +When we try to satisfy the checker +`bugprone-unused-return-value `_, +we sometimes see that we call functions that are unworthy of their return values being checked. +Such functions should be refactored first. + +Also, when we need to document all the possible return values of a function, +it feels strange to have only 1 possible return value. + +Examples: + +The following function `f` and `f2` return always `0`: + +.. code-block:: c + + int f() { + return 0; + } + + int f2() { + int ret = 0; + return ret; + } + +becomes + +.. code-block:: c + + void f() { + return; + } + + void f2() { + int ret = 0; + return; + } + +Note: in above samples, the final `return` statement should be removed as well, +using `readability-redundant-control-flow `_. + +This is useful in following cleanup: +.. code-block:: c + + int g(int i) { + f(); + // ^ Warning: unused return value of f() + if (i > 0) + return 1; + return 0; + } diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.h b/clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.h @@ -0,0 +1,3 @@ +int f11(void); +// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: function 'f11' returns always the same value [readability-useless-return-value] +// CHECK-FIXES: {{^}}void f11(void);{{$}} diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.c b/clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.c new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.c @@ -0,0 +1,252 @@ +// RUN: %check_clang_tidy %s readability-useless-return-value %t + +int f() { + return 0; +} +// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: function 'f' returns always the same value [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value] + +// CHECK-FIXES: {{^}}void f() {{{$}} +// CHECK-FIXES: {{^}} return;{{$}} + +unsigned int f2() { + return 0U; +} +// CHECK-MESSAGES: :[[@LINE-3]]:14: warning: function 'f2' returns always the same value [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value] + +// CHECK-FIXES: {{^}}void f2() {{{$}} +// CHECK-FIXES: {{^}} return;{{$}} + +typedef unsigned int mytype_t; +mytype_t f3() { + return 0U; +} +// CHECK-MESSAGES: :[[@LINE-3]]:10: warning: function 'f3' returns always the same value [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value] + +// CHECK-FIXES: {{^}}void f3() {{{$}} +// CHECK-FIXES: {{^}} return;{{$}} + +const int f4() { + return 0; +} +// CHECK-MESSAGES: :[[@LINE-3]]:11: warning: function 'f4' returns always the same value [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value] + +// CHECK-FIXES: {{^}}const void f4() {{{$}} +// CHECK-FIXES: {{^}} return;{{$}} + +static int f5() { + return 0; +} +// CHECK-MESSAGES: :[[@LINE-3]]:12: warning: function 'f5' returns always the same value [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value] + +// CHECK-FIXES: {{^}}static void f5() {{{$}} +// CHECK-FIXES: {{^}} return;{{$}} + +#define EXIT_SUCCESS 0 + +int f6() { + return EXIT_SUCCESS; //EXIT_SUCCESS +} +// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: function 'f6' returns always the same value [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value] + +// CHECK-FIXES: {{^}}void f6() {{{$}} +// CHECK-FIXES: {{^}} return; //EXIT_SUCCESS{{$}} + +#define NULL 0 + +int f7() { + return NULL; //NULL +} +// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: function 'f7' returns always the same value [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value] + +// CHECK-FIXES: {{^}}void f7() {{{$}} +// CHECK-FIXES: {{^}} return; //NULL{{$}} + +int f8(int i) { + if (i < 0) { + f7(); + return 0; //#1 + } else { + return 0; //#2 + } +} +// CHECK-MESSAGES: :[[@LINE-8]]:5: warning: function 'f8' returns always the same value [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-6]]:5: warning: returns 0 here [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-5]]:5: warning: returns 0 here [readability-useless-return-value] + +// CHECK-FIXES: {{^}}void f8(int i) {{{$}} +// CHECK-FIXES: {{^}} return; //#1{{$}} +// CHECK-FIXES: {{^}} return; //#2{{$}} + +int f9() { + return (0); //(0) +} +// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: function 'f9' returns always the same value [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value] + +// CHECK-FIXES: {{^}}void f9() {{{$}} +// CHECK-FIXES: {{^}} return; //(0){{$}} + +int f10(void); +int f10() { + return 0; +} +// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: function 'f10' returns always the same value [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: function 'f10' returns always the same value [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-4]]:3: warning: returns 0 here [readability-useless-return-value] + +// CHECK-FIXES: {{^}}void f10(void);{{$}} +// CHECK-FIXES: {{^}}void f10() {{{$}} +// CHECK-FIXES: {{^}} return;{{$}} + +// fix-it works in the header file, but not handled by check_clang_tidy.py +// #include "readability-useless-return-value.h" +int f11() { + return 0; +} +// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: function 'f11' returns always the same value [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value] + +// CHECK-FIXES: {{^}}void f11() {{{$}} +// CHECK-FIXES: {{^}} return;{{$}} + +static __attribute__((section("abc"))) int f12() { + return 0; +} +// CHECK-MESSAGES: :[[@LINE-3]]:44: warning: function 'f12' returns always the same value [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value] + +// CHECK-FIXES: {{^}}static __attribute__((section("abc"))) void f12() {{{$}} +// CHECK-FIXES: {{^}} return;{{$}} + +int f13() { + int ret = 0; + return ret; +} +// CHECK-MESSAGES: :[[@LINE-4]]:5: warning: function 'f13' returns always the same value [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value] + +// CHECK-FIXES: {{^}}void f13() {{{$}} +// CHECK-FIXES: {{^}} return;{{$}} + +int f14(int i) { + int ret = 0; + if (i > 0) + return 0; //(f14-1) + if (i > 1) + return ret; //(f14-2) + int ret1 = (0); + if (i > 2) + return ret1; //(f14-3) + return ret; //(f14-4) +} +// CHECK-MESSAGES: :[[@LINE-11]]:5: warning: function 'f14' returns always the same value [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-9]]:5: warning: returns 0 here [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-8]]:5: warning: returns 0 here [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-6]]:5: warning: returns 0 here [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-6]]:3: warning: returns 0 here [readability-useless-return-value] + +// CHECK-FIXES: {{^}}void f14(int i) {{{$}} +// CHECK-FIXES: {{^}} return; //(f14-1){{$}} +// CHECK-FIXES: {{^}} return; //(f14-2){{$}} +// CHECK-FIXES: {{^}} return; //(f14-3){{$}} +// CHECK-FIXES: {{^}} return; //(f14-4){{$}} + +int f15() { + return (int)0; //(f15) +} +// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: function 'f15' returns always the same value [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value] + +// CHECK-FIXES: {{^}}void f15() {{{$}} +// CHECK-FIXES: {{^}} return; //(f15){{$}} + +void g(); + +void g1() { +} + +int g2(int i) { + if (i < 0) + return 0; + else + return 1; +} + +int g3() { + return (0 + 1); +} + +int g4() { + return (0) + 1; +} + +int g5() { + return g4(); +} + +int main() { + return 0; +} + +int g6() { + int ret = 1; + return ret; +} + +int g7() { + int ret = 0; + ret = 1; + return ret; +} + +int g8() { + int ret = 0; + ret = g7(); + return ret; +} + +int g9() { + int ret = 0; + ret = 0; /* Should we add match for this case? */ + return ret; +} + +int g10(int i) { + int ret = 0; + if (i > 0) + return 1; + if (i > 1) + return ret; + int ret1 = 0; + if (i > 2) + return ret1; + return ret; +} + +int g11(int i) { + return i; +} + +int g12() { + int ret = 0; + ret++; + return ret; +} + +int g13() { + int ret = 0; + g11(&ret); + return ret; +} + +int ret = 0; +int g14() { + return ret; +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.cpp @@ -0,0 +1,32 @@ +// RUN: %check_clang_tidy %s readability-useless-return-value %t + +int f() { + return 0; +} +// CHECK-MESSAGES: :[[@LINE-3]]:5: warning: function 'f' returns always the same value [readability-useless-return-value] +// CHECK-MESSAGES: :[[@LINE-3]]:3: warning: returns 0 here [readability-useless-return-value] + +// CHECK-FIXES: {{^}}void f() {{{$}} +// CHECK-FIXES: {{^}} return;{{$}} + +struct Foo { + Foo(int i); + Foo &operator=(int &&other); +}; +Foo tie(int i) { + return Foo(i); +} + +int demangleUnsigned() { + int Number = 0; + tie(Number) = 1; + return Number; +} + +class Foo1 { + virtual unsigned g(void) const; +}; + +unsigned Foo1::g(void) const { + return 0; +}