LukasHanel updated this revision to Diff 321715.
LukasHanel added a comment.
Address review comments, fix C++ unit tests
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96082/new/
https://reviews.llvm.org/D96082
Files:
clang-tools-extra/clang-tidy/readability/CMakeLists.txt
clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.cpp
clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/docs/clang-tidy/checks/readability-useless-return-value.rst
clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.c
clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.cpp
clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.h
Index: clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.h
===================================================================
--- /dev/null
+++ 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);{{$}}
Index: clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.cpp
===================================================================
--- /dev/null
+++ 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;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.c
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/readability-useless-return-value.c
@@ -0,0 +1,247 @@
+// 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;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/readability-useless-return-value.rst
===================================================================
--- /dev/null
+++ 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 <bugprone-unused-return-value.html>`_,
+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 <readability-redundant-control-flow.html>`_.
+
+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;
+ }
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -312,6 +312,7 @@
`readability-uniqueptr-delete-release <readability-uniqueptr-delete-release.html>`_, "Yes"
`readability-uppercase-literal-suffix <readability-uppercase-literal-suffix.html>`_, "Yes"
`readability-use-anyofallof <readability-use-anyofallof.html>`_,
+ `readability-useless-return-value <readability-useless-return-value.html>`_, "Yes"
`zircon-temporary-objects <zircon-temporary-objects.html>`_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -69,6 +69,11 @@
The improvements are...
+ - New :doc:`readability-useless-return-value <clang-tidy/checks/readability-useless-return-value>` check.
+
+ Finds functions that always return `0`, such functions could be ``void``.
+
+
Improvements to include-fixer
-----------------------------
Index: clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.h
===================================================================
--- /dev/null
+++ 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
Index: clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.cpp
@@ -0,0 +1,80 @@
+//===--- 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)).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<FunctionDecl>("function-to-void");
+ if (!MatchedDecl->getIdentifier())
+ return;
+ replaceWithVoid(MatchedDecl);
+
+ const FunctionDecl *ProtoDecl = MatchedDecl->getCanonicalDecl();
+ if (ProtoDecl)
+ replaceWithVoid(ProtoDecl);
+
+ const auto *MatchedReturn =
+ Result.Nodes.getNodeAs<ReturnStmt>("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
Index: clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/ReadabilityTidyModule.cpp
+++ 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<StringCompareCheck>(
"readability-string-compare");
+ CheckFactories.registerCheck<UselessReturnValueCheck>(
+ "readability-useless-return-value");
CheckFactories.registerCheck<readability::NamedParameterCheck>(
"readability-named-parameter");
CheckFactories.registerCheck<NonConstParameterCheck>(
Index: clang-tools-extra/clang-tidy/readability/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/readability/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/readability/CMakeLists.txt
@@ -44,6 +44,7 @@
UniqueptrDeleteReleaseCheck.cpp
UppercaseLiteralSuffixCheck.cpp
UseAnyOfAllOfCheck.cpp
+ UselessReturnValueCheck.cpp
LINK_LIBS
clangTidy
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
