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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.03.2022 15:28, Bertrand Marquis wrote:
>> 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 first tried to clarify what I'm concerned about, but then said that
apparently there is no reason to be concerned. I'm sorry if this didn't
come across quite clear enough. Bottom line - no request for any
change here.

Jan

> 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
>>
>