snitch-org / snitch

Lightweight C++20 testing framework.
Boost Software License 1.0
257 stars 7 forks source link

PVS analyzer founds issue in "CHECK" macro #85

Closed victimsnino closed 1 year ago

victimsnino commented 1 year ago

Hey! Currently i'm getting PVS warnings related to "CHECK" macro. I've debugged a bit. Looks like issue is there https://github.com/cschreib/snitch/blob/main/include/snitch/snitch.hpp#L2324 and other similar places.

Repro:

CHECK(some_val == 2);

Warning:

It's odd to compare a bool type value with a value of 2.

Short repro: https://godbolt.org/z/a6EnPoqvT

Looks like __VA_ARGS__ should be placed in ()

cschreib commented 1 year ago

Hi there, and sorry for the slow response, I'm on holiday this week.

Your reproduction code isn't quite like the real code; in snitch, the expression decomposer operator<= doesn't return a bool, but an unary_expression. This funky operator overloading is done (as in Catch2, doctest, or Boost UT) to enable expression decomposition. The __VA_ARGS__ cannot be placed within parentheses in this case, because it would prevent this from working. When the macro expands, it will "look" as if the expression is weird, but it is actually as intended.

I assume PVS studio isn't able to see through the trick, so it reports a false positive here. Do you have a way to silence this warning on your end?

victimsnino commented 1 year ago

I've double checked and see your trick. It is not a problem for me to silence warning, just wanted to share.

BTW, looks like catch2 W/A it with help of pragmas

__pragma(warning(push)) catchAssertionHandler.handleExpr(Catch::Decomposer() <= call_count == 0);
__pragma(warning(pop))
cschreib commented 1 year ago

__pragma(warning(push)) and __pragma(warning(pop)) don't silence warnings on their own, they just introduce a new warning scope. You then need __pragma(warning(disable: ...)) to actually silence a warning. For example:

__pragma(warning(push))
__pragma(warning(disable: 4127))
// your code here: warning 4127 is disabled
__pragma(warning(pop))

// other code outside of the warning scope: warning 4127 is no longer disabled

See the Microsoft documentation (since __pragma(...) is a Microsoft extension, I assume you're using MSVC). snitch already uses this to silence some warnings (using _Pragma(...) instead, which is standard C).

Since you're able to silence the PVS warning on your end, I'll mark this issue as closed. You may want to report the false-positive to the PVS team.