[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D41655#980672, @khuttun wrote:

> The checker reports 7 warnings on LLVM + Clang code bases, all on std::unique_ptr::release:
>
> lib/Bitcode/Reader/BitReader.cpp:114:3
>
> - release() called on moved-from unique_ptr
> - no harm, just unnecessary


Agreed.

> lib/ExecutionEngine/ExecutionEngine.cpp:149:7
> 
> - release() called before erasing unique_ptr from a container
> - false positive?

False positive.

> lib/Target/AMDGPU/GCNIterativeScheduler.cpp:196:5
> 
> - release() called before assigning new pointer to unique_ptr
> - false positive?

False positive, but a bad code smell given that the assignment operator will perform the release.

> lib/AsmParser/LLParser.cpp:855:3
> 
> - get() + release() could be replaced with release()
> 
>   tools/clang/lib/Lex/ModuleMap.cpp:791:3
> - false positive?

False positive.

> tools/clang/tools/extra/clangd/Compiler.cpp:61:3
> 
> - get() + release() could potentially be replaced with release()?

False positive.

> unittests/Support/Casting.cpp:144:3
> 
> - release() called to avoid calling delete on a pointer to global object
> - false positive?

False positive.

>From what I can tell of these reports, they almost all boil down to ignoring the call to `release()` because the pointer is no longer owned by the `unique_ptr`. This is a pretty reasonable code pattern, but it also seems reasonable to expect users to cast the result to `void` to silence the warning, so I think this is fine. Have you checked any other large C++ code bases, like Qt or boost?


https://reviews.llvm.org/D41655