feat: enforcing MISRA compliant Boolean values

[anshgoyalevil]

This PR adds a literal to boolean style check.

bool x = 1;  // Warning: Boolean variable assigned a numeric literal '1'. Consider using 'true' instead.
bool y = 0;  // Warning: Boolean variable assigned a numeric literal '0'. Consider using 'false' instead.

ref: https://github.com/commaai/panda/pull/1876, https://github.com/commaai/panda/issues/2105

[okias]

damn, I'm too slow. Anyway, shouldn't this go into addons/misra.py ? I would guess Rule 23.6

[anshgoyalevil]

Thanks for the suggestion, @okias. I am fairly new to this MISRA thing. I just read the discussion in the thread https://github.com/commaai/panda/pull/1876 and this way looked like what adding the check into ustream meant.

Can you please help identify if that's the right way to do that? Adding a check into upstream without explicitly defining an addon code.

[okias]

In general, a C++ solution like the one you wrote looks better, but I understood it as part of the MISRA plugin—something like this: https://github.com/okias/cppcheck/commit/6b3d84a11ecdc67c038c276576eea70b9bb596c6. The code doesn't detect the initial bool x = 1;, only bool x; x = 1;. This was also my first attempt with cppcheck..

[anshgoyalevil]

Thanks. Now I get the point. I would implement it the MISRA plugin way if this one is rejected by the maintainers. Can you please review it @danmar

[danmar]

Hi, sorry for the late review. I spontanously feel this might be too "stylistic" in nature. It's not to find "buggy" code, is it? What bugs could it lead to? Normally I like if "style" checkers have a motivation to prevent some possible bugs. For instance; unread variables can for instance indicate an overlook.

I wouldn't normally write bool x=1; myself explicitly.

In my humble opinion it does not violate MISRA 23.6 but it does violate 10.3 and Cppcheck Premium writes a warning about it.

[danmar]

The implementation: I don't have major complaints. It's pretty good. But some test cases is needed and also I think a ticket should be added in trac and the release notes would need to mention the new checker..

[anshgoyalevil]

Thanks for the inputs. This would not help prevent bugs to the code but would help increase readability at some places. I would add the required test cases.

Can you please elaborate that trac thing. How can I file a ticket out there?

[danmar]

This would not help prevent bugs to the code but would help increase readability at some places. I would add the required test cases.

well.. I don't know why anybody would WANT to do this.. but still I don't have a very good feeling about this. This is not the high prio findings users want to see. Could you try to write a checker that looks for "mistakes" somehow? I believe trac has many suggestions as inspiration.

https://trac.cppcheck.net

to get an account on trac you'll need to send a htpasswd hash to me. You can generate a htpasswd hash online, google "htpasswd hash generator"

[tralamazza]

This looks to be MISRA 10.x territory. Why is the check restricted to literal 0 and 1? Any prvalue zero of an integral, floating point and unscoped enum plus nullptr converts to false, any other value to true. I don't see a use case for initializing a bool from any other literal than true or false.

https://cpp.godbolt.org/z/qW4K1facc

[wienans]

Checked the Misra Plugin and currently it is checked here if a bool gets assigned a value bigger than 0 or 1 https://github.com/danmar/cppcheck/blob/85423bda21cfba3f16c16fb2db52cf6f214f59e2/addons/misra.py#L2328-L2329

This looks to be MISRA 10.x territory. Why is the check restricted to literal 0 and 1? Any prvalue zero of an integral, floating point and unscoped enum plus nullptr converts to false, any other value to true. I don't see a use case for initializing a bool from any other literal than true or false.

https://cpp.godbolt.org/z/qW4K1facc

And additionally i checked your code against the Warnings

10.3 was found for

bool bint_t = -1;
bool bflt_f = 0.f;
bool bflt_t = 20.f;

and the following was false negative:

bool bint_f = 0; // expected as 0 and 1 are allowed as value, but actually the essential type changes

enum Foo {
    BAR = 0,
    BAZ = 7
};

bool benuml_f = BAR; // semi expected as 0 and 1 are allowed for the value but actually cppcheck does not deuce it to 0 or 1
bool benuml_t = BAZ;

bool bnull_f{NULL}; // only direct init
bool bnull_t = &bnull_f;

This is because the Essential Type https://github.com/danmar/cppcheck/blob/85423bda21cfba3f16c16fb2db52cf6f214f59e2/addons/misra.py#L543 for the Enums and the {NULL} as well as &bnull_f is not handled here and returns None at the end of the function, even if they have a valueType assigned to them. Similar is true for the keywords true and false they both have a value type assigned to them in the dump file but they both also return None from the Essential Type function and therefore the check stops due to one of the sides having a None Type.

And maybe i am tripping but seeing https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/tools/-/blob/main/misra_c_2012__headlines_for_cppcheck%20-%20AMD1+AMD2.txt?ref_type=heads#L180 i think all of them should be a 10.3 as @tralamazza says. It states that it shouldn't be narrower, where you could debate if you directly assign 0 and 1 is okay, but i think all other parts should fail as the essential type changes, which is the second part of the rule.

[danmar]

if you change misra.py rule 10.3 so that it warns I am not against it.

[danmar]

I have the feeling that C89 does not have a "bool" type. And one possible way to use bools would be:

typedef int bool;
#define true 1
#define false 0
bool x = true;

my guess this is why misra.py is written as it is.

[wienans]

I have the feeling that C89 does not have a "bool" type. And one possible way to use bools would be:

typedef int bool;
#define true 1
#define false 0
bool x = true;

my guess this is why misra.py is written as it is.

Actually true 🤔 stdbool was added in C99. Maybe dependent on Code Standard for C89 allow 0 and 1 and for C99 not?

[danmar]

Maybe dependent on Code Standard for C89 allow 0 and 1 and for C99 not?

Sounds good to me.