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

[Nathan James via Phabricator via cfe-commits]

njames93 requested changes to this revision.
njames93 added a comment.
This revision now requires changes to proceed.

This check, more specifically the fixes, seem quite dangerous. Changing function signatures is highly likely to cause compilation issues. 
For a starters this check doesn't appear to examine call sites of the function to ensure they don't use the return value.
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.
>From these 2 issues, this shouldn't trigger on functions with external linkage.

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.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96082