Fix #13876 (Document cstyleCast) [ci skip]

[danmar]

I document the checker.

The documentation is written in a markdown file so we can link directly to github later. We don't need to upload to a website. But well if you have some alternative better ideas feel free to share..

If the documentation looks OK I will review cstyleCast checker to ensure it works as intended.

One observation is that as far as I see the existing cstyleCast checker does not warn about this C style cast that has a loss of constness:

const int c = 123;
int * p = (int *)&c;

However this may be OK:

• probably that should be a separate checker, it makes sense to warn about this for plain old C code also.
• some compilers detect this anyway , for instance clang shows Wcast-qual warnings.
• There is checking for this problem in Misra C.

[Copilot]

Pull Request Overview

This PR adds documentation for the cstyleCast checker to clearly explain its motivation, philosophy, examples, and how to enable it.

• Introduces the markdown documentation for the cstyleCast checker.
• Provides motivation, compliant versus non-compliant examples, and instructions on how to fix unsafe casts.

[firewave]

Having examples in the documentation obviously helps but it provides incomplete code which might be misleading.

We also have the examples which would provide way more details and an actual sample file to analyze. Maybe we should merge that with the documentation.

[danmar]

Having examples in the documentation obviously helps but it provides incomplete code which might be misleading.

I made the code complete

We also have the examples which would provide way more details and an actual sample file to analyze. Maybe we should merge that with the documentation.

I do like to have the code directly in the documentation. If we merge it somehow I am not against it basically. But personally I would drop the examples folder and just provide the documentation.

[firewave]

Having the examples separate helps making sure that the code is valid and is always detected. Also people use this to test their setups. We could inject the examples via a script.

[danmar]

We could inject the examples via a script.

ok

[firewave]

Having the examples separate helps making sure that the code is valid and is always detected. Also people use this to test their setups. We could inject the examples via a script.

This is not just personal preference (i.e. avoid duplicated/manual work) but based on actual feedback I have seen with Cppcheck and clang-tidy (they have/had several broken code examples in documentation) - mostly from first-time users.

I am not blocking this PR but it is something we should consider moving forward.

We could also leverage the effort of documenting/adding examples for the checks to fix the holes in the unit testing - see https://trac.cppcheck.net/ticket/12232.

[danmar]

@chrchr-github please review the checker updates. It should now match the documentation better. It's now based on ValueType and that should be more reliable.

[chrchr-github]

It seems that we no longer warn for incomplete code, and in some cases that are considered "safe". Did someone complain about too many warnings from this check?

[danmar]

It seems that we no longer warn for incomplete code, and in some cases that are considered "safe". Did someone complain about too many warnings from this check?

No we did not get complaints. I just picked this checker because it is the checker that generates most warnings in debian and if people want to see such documentation I thought it was best to start from the top..

Basically.. if there is incomplete code which looks unsafe I feel we can warn.. and it's then possible to fix such warning by including proper headers etc. So is there some specific case you are thinking about?

[chrchr-github]

Basically.. if there is incomplete code which looks unsafe I feel we can warn.. and it's then possible to fix such warning by including proper headers etc. So is there some specific case you are thinking about?

Some test cases were changed to include the definition of a base class (testother.cpp), but I'm not sure if that is required to get a warning.

There are no more warnings for #12446 (cast from void*). I think it is still better to use static_cast here, so it is clear what's going on.

[danmar]

Some test cases were changed to include the definition of a base class (testother.cpp), but I'm not sure if that is required to get a warning.

Yes that was required. I have fixed that and provide this test case that has unknown types:

https://github.com/danmar/cppcheck/pull/7539/files#diff-ff2c5f41c62937b3b6b04ead271c88d15f5d67d40156a4a1121faf8ca7e9217cR1931

[danmar]

There are no more warnings for #12446 (cast from void*). I think it is still better to use static_cast here, so it is clear what's going on.

imho that is 100% about readability. A static_cast does not provide any safety guarantee beyond C casts right (when casting from void*)?

[chrchr-github]

There are no more warnings for #12446 (cast from void*). I think it is still better to use static_cast here, so it is clear what's going on.

imho that is 100% about readability. A static_cast does not provide any safety guarantee beyond C casts right (when casting from void*)?

Yes, but at least with static_cast you can be sure that const is not cast away, e.g. when the source type is changed later.
cstylecast already has the lowest severity "style". Maybe we should have a new ID dangerousCstyleCast as a warning in case we know that something bad can happen.

[danmar]

Yes, but at least with static_cast you can be sure that const is not cast away, e.g. when the source type is changed later.

yes sure. you can get warnings with the cstyle cast for loss of constness but the static_cast is better.

Maybe we should have a new ID dangerousCstyleCast as a warning in case we know that something bad can happen.

I think it's a good idea to split it up