mitigated most bugprone-unused-return-value clang-tidy warnings in test code when we enable it for *all* functions#6673
Conversation
|
See llvm/llvm-project#85913 for a discussion about the stream insertion operator "false positives". |
| const char code[] ="#elif A\n" | ||
| "A,a<b<x0\n"; | ||
| tokenize(code); | ||
| (void)tokenize(code); |
There was a problem hiding this comment.
These could also be written as ASSERT_NO_THROW() and my changes here are as inconsistent as the existing code.
There was a problem hiding this comment.
We could just disallow using (void) and that would force us to use ASSERT_NO_THROW. I think I might do that in a follow-up PR to make things consistent.
There was a problem hiding this comment.
I think there are also some unnecessary return values in the test code which could also be cleaned up in a follow-up.
There was a problem hiding this comment.
I filed https://trac.cppcheck.net/ticket/13006 about detecting functions which always have their result ignored.
firewave
force-pushed
the
tidy-unused-ret
branch
2 times, most recently
from
5e679fa to
e00d83e
Compare
…test code when we enable it for *all* functions
firewave
force-pushed
the
tidy-unused-ret
branch
from
be10d7e to
a520374
Compare
There was a problem hiding this comment.
An observation I have is that I believe we don't want to have warnings about all unused return values in all our code.
99% of the fixes here seems to be "false positives" we are really not interested in the return value.. but there are some cases where it does make sense to check the result.
There is Misra C++ 2023 0.1.2 rule that also complains about this and I find it way too noisy to be used.
That's why I only apply it to the test code.
I think a lot of what I suppressed here has return codes for no good reason. I will explore that in a follow-up now that usage/suppression has been clarified by these fixes. |
danmar
left a comment
danmar
left a comment
There was a problem hiding this comment.
That's why I only apply it to the test code.
Good. I approve this with that limitation.
2 participants
We should not have any unchecked returns within the test code to make sure everything is working as expected (there might be a case that should also apply to the production code).
Unfortunately enabling all functions is abusing this check and it will produce a lot of "false positives" (e.g.
erase()/insert()of containers or usage of stream insertion operators) so this cannot be enabled by default. Sincellvm:Regexdoes not support negative lookahead we cannot configure the check to exclude functions.