Re: [RFC PATCH] xen/build: Add cppcheck and cppcheck-html make rules

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Jan,

> On 25 Mar 2022, at 15:10, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 25.03.2022 13:57, Bertrand Marquis wrote:
>>> On 25 Mar 2022, at 12:43, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> On 24.03.2022 12:04, Bertrand Marquis wrote:
>>>> --- a/xen/include/xen/kconfig.h
>>>> +++ b/xen/include/xen/kconfig.h
>>>> @@ -8,6 +8,10 @@
>>>> * these only work with boolean option.
>>>> */
>>>> 
>>>> +/* cppcheck is failing to parse the macro so use a dummy one */
>>>> +#ifdef CPPCHECK
>>>> +#define IS_ENABLED(option) option
>>>> +#else
>>>> /*
>>>> * Getting something that works in C and CPP for an arg that may or may
>>>> * not be defined is tricky.  Here, if we have "#define CONFIG_BOOGER 1"
>>>> @@ -27,5 +31,6 @@
>>>> * otherwise.
>>>> */
>>>> #define IS_ENABLED(option) config_enabled(option)
>>>> +#endif
>>> 
>>> What are the consequences of this workaround on the code actually
>>> being checked? Does this perhaps lead to certain portions of code
>>> being skipped while checking?
>> 
>> Cppcheck is not optimising the code but looking at the syntax so the
>> consequence here is that cppcheck is checking some code which might
>> be optimised out by the compiler. This is not optimal but still it should
>> analyse properly the code.
> 
> Aren't you saying so merely because all uses of IS_ENABLED() in our
> sources so far are in if()? It would seem to me that when used in #if
> (as can be found in Linux, which hence means could be the case in our
> tree as well sooner or later) sections of code might either be skipped
> or syntax errors may result. Or wait - IS_ENABLED() does itself kind
> of rely on the respective CONFIG_* to expand to a numeric value; when
> expanding to a string, I guess the compiler would also warn about the
> resulting construct when used in if() (and reject any uses with #if).

I am not quite sure I am following what you are saying (or asking).

I say that most use cases are if (IS_ENABLED(x)) so from the syntax point
of view it is ok to not do exactly as IS_ENABLED really does. And
cppcheck is checking the code not the result.
Now it would be better to do it right but the point of the patch is to enable
cppcheck not make it perfect on the first shot.

Cheers
Bertrand

> 
> Jan
>