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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D96082#2561806 <https://reviews.llvm.org/D96082#2561806>, @steveire wrote:

> In D96082#2550468 <https://reviews.llvm.org/D96082#2550468>, @LukasHanel wrote:
>
>> In D96082#2549943 <https://reviews.llvm.org/D96082#2549943>, @steveire wrote:
>>
>>> In D96082#2545807 <https://reviews.llvm.org/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?
>
> I'm not sure this check meets the general applicability and usefulness threshold to be part of clang-tidy. The warning alone isn't actionable. Someone wanting to take action on the warning would have to write their own clang tidy check to make the change across their codebase. Add that to the false-positive issues I mentioned before.
>
> Does anyone have any other thoughts?

I agree. I'm not certain such a check could ever be generally applicable. For example, some functions return zero always because it's part of the API design. e.g.,

  struct Base {
    virtual int foo() { return 0; /* Degenerate case */ }
  };
  
  struct Derived : Base {
    int foo() override; // More useful implementation
  };
  
  // Or
  namespace std {
  template <>
  class numeric_limits<MyAwesomeType> {
  public:
    ...
    static constexpr MyAwesomeType lowest() noexcept { return 0; /* implicit conversion to the return type */ }
    ...
  };

Also, there's nothing particularly interesting about `0` as opposed to any other constant return value.

A somewhat similar check that would be interesting is a function that returns the same value on all control paths, as that may signify a logical error on the part of the programmer. e.g.,

  int foo() {
    if (whatever)
      return 0;
   ... code without return statements ...
    return 0;
  }
  
  // Or
  int bar() {
    if (whatever)
      return foo();
    else
      ...
    ...
    return foo();
  }

but I'd want this check to ignore functions that only have a single `return` statement in them in order to reduce noise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96082