[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 <https://reviews.llvm.org/D96082#2566984>, @steveire wrote:

> In D96082#2565339 <https://reviews.llvm.org/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