Re: [PATCH 0/9] MISRA C 2012 8.1 rule fixes

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.06.2022 16:27, Bertrand Marquis wrote:
> Hi,
> 
>> On 22 Jun 2022, at 15:10, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 22.06.2022 15:55, Bertrand Marquis wrote:
>>> Hi Jan,
>>>
>>>> On 22 Jun 2022, at 14:01, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>
>>>> On 22.06.2022 14:55, Michal Orzel wrote:
>>>>> On 22.06.2022 12:25, Jan Beulich wrote:
>>>>>> On 20.06.2022 09:02, Michal Orzel wrote:
>>>>>>> This series fixes all the findings for MISRA C 2012 8.1 rule, reported 
>>>>>>> by
>>>>>>> cppcheck 2.7 with misra addon, for Arm (arm32/arm64 - target 
>>>>>>> allyesconfig).
>>>>>>> Fixing this rule comes down to replacing implicit 'unsigned' with 
>>>>>>> explicit
>>>>>>> 'unsigned int' type as there are no other violations being part of that 
>>>>>>> rule
>>>>>>> in the Xen codebase.
>>>>>>
>>>>>> I'm puzzled, I have to admit. While I agree with all the examples in the
>>>>>> doc, I notice that there's no instance of "signed" or "unsigned" there.
>>>>>> Which matches my understanding that "unsigned" and "signed" on their own
>>>>>> (just like "long") are proper types, and hence the omission of "int"
>>>>>> there is not an "omission of an explicit type".
>>>>>>
>>>>> Cppcheck was choosed as a tool for MISRA checking and it is considering 
>>>>> it as a violation.
>>>>
>>>> Which by no means indicates that the tool pointing out something as a
>>>> violation actually is one.
>>>>
>>>>> It treats unsigned as an implicit type. You can see this flag in cppcheck 
>>>>> source code:
>>>>>
>>>>> "fIsImplicitInt = (1U << 31), // Is "int" token implicitly added?"
>>>>
>>>> Neither the name of the variable nor the comment clarify that this is about
>>>> the specific case of "unsigned". As said there's also the fact that they
>>>> don't appear to point out the lack of "int" when seeing plain "long" (or
>>>> "long long"). I fully agree that "extern x;" or "const y;" lack explicit
>>>> "int".
>>>
>>> I am a bit puzzled here trying to understand what you actually want here.
>>>
>>> Do you suggest the change is ok but you are not ok with the fact that is 
>>> flagged
>>> as MISRA fix even though cppcheck is saying otherwise ?
>>
>> First of all I'd like to understand whether what we're talking about here
>> are actually violations (and if so, why that is). Further actions / requests
>> depend on the answer.
> 
> I would say yes but this would need to be confirmed by Roberto I guess.
> In any case if we think this is something we want and the change is right
> and cppcheck is saying that it solves a violation, I am wondering what is
> the point of discussing that extensively this change.

Because imo a patch shouldn't be committed with a misleading description,
if at all possible. Even less so several such patches in one go.

Jan