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

Mon Feb 8 22:37:40 PST 2021

LukasHanel added a comment.

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?

> 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