[PATCH] D96080: format

[Lukas Hänel via Phabricator via cfe-commits]

LukasHanel created this revision.
Herald added a subscriber: nullptr.cpp.
LukasHanel requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D96080

Files:
  clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.cpp


Index: clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.cpp
@@ -30,14 +30,13 @@
   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(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(


Index: clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/UselessReturnValueCheck.cpp
@@ -30,14 +30,13 @@
   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(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(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96082: [clang-tidy] Add 'readability-useless-return-value' check

[Lukas Hänel via Phabricator via cfe-commits]

LukasHanel created this revision.
Herald added subscribers: nullptr.cpp, xazax.hun, mgorny.
LukasHanel requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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/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]]:5: 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: wa

[PATCH] D96082: [clang-tidy] Add 'readability-useless-return-value' check

[Lukas Hänel via Phabricator via cfe-commits]

LukasHanel updated this revision to Diff 321580.
LukasHanel edited projects, added clang-tools-extra; removed clang.
LukasHanel added a comment.
Herald added a project: clang.

Add the real changes


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/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]]:5: 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-usel

[PATCH] D96082: [clang-tidy] Add 'readability-useless-return-value' check

[Lukas Hänel via Phabricator via cfe-commits]

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

[PATCH] D96082: [clang-tidy] Add 'readability-useless-return-value' check

[Lukas Hänel via Phabricator via cfe-commits]

LukasHanel marked 9 inline comments as done.
LukasHanel added a comment.

Thanks for the review!




Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability-useless-return-value.rst:43
+int ret = 0;
+return;
+  }

Eugene.Zelenko wrote:
> Return is redundant. See readability-redundant-control-flow.
My new checker will not remove the return statement. I added a new comment to 
the readme to recommend usage of the other checker as well.
Should I extend the fix-it to remove also the `return` statement?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96082/new/

https://reviews.llvm.org/D96082

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96082: [clang-tidy] Add 'readability-useless-return-value' check

[Lukas Hänel via Phabricator via cfe-commits]

LukasHanel added a comment.

Hi, thanks for discussing my proposal!
Although I think it can stand as is, I was looking for feedback:

- Is the name good?
- Is the `readability` group  good? Or better in `misc`?
- too slow, too fast?
- More precision required?
- Usefulness of the fix-it's
- Should I add options?

Anyhow, last year I was refactoring our companies code base and often manually 
searched for functions that would be identified by such checker. And as you 
say, I did not implement the corner cases yet.
Clang-tidy is a really easy environment to write such checkers, Kudos to all 
the contributors and maintainers.

On your comments:

In D96082#2544836 , @njames93 wrote:

> This check, more specifically the fixes, seem quite dangerous. Changing 
> function signatures is highly likely to cause compilation issues.

This seems to be a general issue with clang-tidy's fixes.
It seems some fixes are more of a gimmick and shouldn't be used without 
supervision.
I did not find any other checker that changed the function signature, is there 
any guidance against this?

> For a starters this check doesn't appear to examine call sites of the 
> function to ensure they don't use the return value.

I have considered this, but it would double the complexity of the checker, so 
did not work on it yet.
Yes, it is a good safety net.
However, it could also hide two issues:

1. Inconsistent use (or not use) of the return value - it is actually a sign of 
the return value being useless (Note: this is a common checker for commercial 
static analyzers).
2. Propagation of the useless return value, let's say `b` checks useless return 
value of `a`, and returns it, but `c` does not check return value of `b`.

  int a() { return 0; } // "useless"
  int b() { return a(); } // "checked"
  int c() {
  b();  // not checked
 // or even
 int ret = b();// "checked"
 if (ret) {
 printf("something went wrong"); // or maybe the return value was 
useless in the first place
 }
 //...
  }

Regarding the propagation, anyhow, such refactoring needs to be done in steps: 
you fix one function, then the linter will highlight the next function.

> There's also the case where the function is used as a callback, this can't 
> change the signature as the callback type would then mismatch.

Yes, we had this case as well.
I wonder how I could detect this.
I guess I need the call sites.

1. List Item
2. Or a configuration for this checker of functions to ignore.
3. Or somehow annotate functions to ignore this checker.

> From these 2 issues, this shouldn't trigger on functions with external 
> linkage.

From my experience, this is actually more a problem for external functions.
Static functions tend to less have such issues because the deveoloper can more 
easily verify the callers.

> There's the case where the return value is based on some preprocessor 
> constant. You are showing how it handles NULL and EXIT_SUCCESS, however what 
> if the macro is called BUILT_WITH_FEATURE_X, that's likely a build setting 
> and shouldn't be triggered upon. Obviously there's no hard and fast way to 
> determine what's a dependent macro and what isn't, so either never trigger on 
> returns where the value is a macro, or have a list of allowed macros, 
> defaulted to containing the likes of NULL.

Curious case of the command-line defined return-value macro. :)
When playing with this part, I found that clang-tidy sees the code after 
preprocessing.
So I never saw that I was handling NULL or EXIT_SUCCESS, I only saw `0`.
Clang-tidy must know about build-time macros from the compile_commands.json 
data base, otherwise it would scream.
Hmm, I start to see what you mean.

> With all being said, I feel that while the diagnosing of these functions can 
> have some value, an automated fix probably isn't a good idea.

Thanks.

Will the CI run my checker on the llvm code base?
I found several issues locally, I will figure out how to best share those 
findings.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96082/new/

https://reviews.llvm.org/D96082

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96082: [clang-tidy] Add 'readability-useless-return-value' check

[Lukas Hänel via Phabricator via cfe-commits]

LukasHanel added a comment.

In D96082#2549943 , @steveire wrote:

> In D96082#2545807 , @LukasHanel 
> wrote:
>
>> Hi, thanks for discussing my proposal!
>>
>> - Usefulness of the fix-it's
>
> I agree with @njames93 that this check is dangerous. Even if you extended it 
> to port callExprs, that would only work in translation units which can see 
> the definition.

Are you saying I should just remove the fix-its altogether?
Or, put them under some option that is off by default?

> In addition to that practical problem, there are some functions which 
> deliberately always return the same value and you don't have a way to 
> distinguish them other than by adding an option to ignore them. It might make 
> the checker a bit of a burden. The kinds of functions I'm referring to 
> include functions which give a name to an otherwise-magic number (`int 
> sentinalValue() { return 0; }`), virtual methods (I see you already handle 
> those), Visitors, dummy implementations of APIs which may not be implemented 
> on a platform, CRTP interfaces, there may be others.



>> Anyhow, last year I was refactoring our companies code base and often 
>> manually searched for functions that would be identified by such checker. 
>> And as you say, I did not implement the corner cases yet.
>
> Does this mean that you didn't try to run this tool on a real codebase? (ie 
> your company code is already changed and doesn't need it anymore). You may 
> run into the cross-TU issue if you run it on a codebase.

Running on my own code base is the next step. clang-tidy is not well integrated 
there yet.
Running on llvm-project, it got me to implement several filters:

- 
https://github.com/llvm/llvm-project/blob/main/llvm/lib/Demangle/MicrosoftDemangle.cpp#L923
 : assignment to std:tie()
- e.g. 
https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/CodeGen/FastISel.h#L473
 virtual methods

In addition, I saw the following:
https://github.com/llvm/llvm-project/blob/892e4567e1357ee10ef67ee6dfbe45aeded9d2dc/llvm/lib/Support/regcomp.c#L1180
Clearly not used most of the time, but the macro REQUIRE uses seterr() in an 
expression. However, a void function call creates a statement, not an 
expression.

doRegionSplit:
https://github.com/llvm/llvm-project/blob/892e4567e1357ee10ef67ee6dfbe45aeded9d2dc/llvm/lib/CodeGen/RegAllocGreedy.cpp#L1965
https://github.com/llvm/llvm-project/blob/892e4567e1357ee10ef67ee6dfbe45aeded9d2dc/llvm/lib/CodeGen/RegAllocGreedy.cpp#L1862
^ Replace here with `call(); return 0;`.
Like here:
https://github.com/llvm/llvm-project/blob/892e4567e1357ee10ef67ee6dfbe45aeded9d2dc/llvm/lib/CodeGen/RegAllocGreedy.cpp#L2826

https://github.com/llvm/llvm-project/blob/892e4567e1357ee10ef67ee6dfbe45aeded9d2dc/llvm/lib/CodeGen/CodeGenPrepare.cpp#L6887
Used 4 times in a private class, with an assert that warns about the single 
use. I guess it is a magic number with an assert, so somewhat useful, but still 
6 years that nobody added more transitions.

>> It seems some fixes are more of a gimmick and shouldn't be used without 
>> supervision.
>
> I think you might be right. Do you have a list of such checks? Maybe they 
> should be removed.

https://clang.llvm.org/extra/clang-tidy/checks/readability-inconsistent-declaration-parameter-name.html

> changing the parameter names, but its declaration in header file is not 
> updated.

For us it fixed things in the wrong order. The header file were polished 
doxygen resembling the user facing documentation or API standard.
In the function definition, we used parameter names that helped in the 
implementation. Those should be changed, not the ones in the header file. This 
should be an option IMHO.

https://clang.llvm.org/docs/DiagnosticsReference.html#wasm-operand-widths
Can be missing context, it changes % to %w, which is not always what you want.

https://clang.llvm.org/docs/DiagnosticsReference.html#wformat-type-confusion 
(not sure which one actually)
%08lx, --> %08llx, but then does it compile in 32-bit 64bit? Maybe should be 
replaced by PRIx64.

Well, mostly, I was set back by the broken formatting after fix-its, and also 
the sheer number of fix-its and related options.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96082/new/

https://reviews.llvm.org/D96082

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D96082: [clang-tidy] Add 'readability-useless-return-value' check

[Lukas Hänel via Phabricator via cfe-commits]

LukasHanel updated this revision to Diff 324132.
LukasHanel added a comment.

Handle the case of a global variable being the "return value"


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,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]

[PATCH] D96082: [clang-tidy] Add 'readability-useless-return-value' check

[Lukas Hänel via Phabricator via cfe-commits]

LukasHanel added a comment.

In D96082#2566984 , @steveire wrote:

> In D96082#2565339 , @aaron.ballman 
> wrote:
>
>> A somewhat similar check that would be interesting is a function that 
>> returns the same value on all control paths
>
> I think we shouldn't try to design a new, different check in the comments of 
> this MR.
>
> I think it would be better to finalize what to do with this one.
>
> Request further work to also change expressions in all affected TUs? Or close?

Hi,
I finally managed to run this checker on my own code base and I run into all 
the above problems:

1. There are some sloppy areas in the code that this checker nicely highlights.
2. Many false-positives for example with callbacks or involving preprocessor
3. Some questionable suggestions, like where it is breaking code-symmetry with 
a set of handlers that have the same signatures but some always return 0.

In conclusion for this checker:

- it's a good way to spot sloppy areas in the code.
- You couldn't run it in -Werror mode to enforce clean code.
- Yes, I could filter more FP/noise, e.g. functions that have a single return 
statement in the body; plus maybe assert.
- However, if you have a policy to use the return value of all functions, this 
checker can be a good way to clean the code first.

I am ok to close this one, I will park it somewhere.

In the meantime, I try to spin up some fixes for the false positives that I see 
with other checkers, as mentioned above.

As a follow-up to this checker, clang -Wdocumentation does not understand the 
`@retval` command.
I was thinking of adding a new clang-tidy checker for this to verify/complete 
the list of documented return values.
In this case where the documentation does not say `@retval 0 always`, than I 
will come back to the checker here, suggest make the function void or add the 
"always" to the text :).
The work with the useless-return-value was a study towards this new @retval 
checker.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96082/new/

https://reviews.llvm.org/D96082

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits