[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 <https://reviews.llvm.org/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