Re: [PATCH 1/2] docs/misra: introduce rules.rst

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.05.2022 01:16, Stefano Stabellini wrote:
> On Fri, 27 May 2022, Jan Beulich wrote:
>> On 26.05.2022 21:57, Stefano Stabellini wrote:
>>> On Thu, 26 May 2022, Bertrand Marquis wrote:
>>>>> On 26 May 2022, at 11:15, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>> On 26.05.2022 11:54, Bertrand Marquis wrote:
>>>>>>> On 26 May 2022, at 10:43, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>>>>> On 26.05.2022 03:02, Stefano Stabellini wrote:
>>>>>>>> On Wed, 25 May 2022, Julien Grall wrote:
>>>>>>>>> On 25/05/2022 01:35, Stefano Stabellini wrote:
>>>>>>>>>> +- Rule: Dir 4.7
>>>>>>>>>> + - Severity: Required
>>>>>>>>>> + - Summary: If a function returns error information then that error
>>>>>>>>>> information shall be tested
>>>>>>>>>> + - Link:
>>>>>>>>>> https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/D_04_07.c
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ... this one. We are using (void) + a comment when the return is 
>>>>>>>>> ignored on
>>>>>>>>> purpose. This is technically not-compliant with MISRA but the best we 
>>>>>>>>> can do
>>>>>>>>> in some situation.
>>>>>>>>>
>>>>>>>>> With your proposed wording, we would technically have to remove them 
>>>>>>>>> (or not
>>>>>>>>> introduce new one). So I think we need to document that we are 
>>>>>>>>> allowing
>>>>>>>>> deviations so long they are commented.
>>>>>>>>
>>>>>>>> Absolutely yes. All of these rules can have deviations as long as they
>>>>>>>> make sense and they are commented. Note that we still have to work out
>>>>>>>> a good tagging system so that ECLAIR and cppcheck can recognize the
>>>>>>>> deviations automatically but for now saying that they need to be
>>>>>>>> commented is sufficient I think.
>>>>>>>>
>>>>>>>> So I'll add the following on top of the file:
>>>>>>>>
>>>>>>>> """
>>>>>>>> It is possible that in specific circumstances it is best not to follow 
>>>>>>>> a
>>>>>>>> rule because it is not possible or because the alternative leads to
>>>>>>>> better code quality. Those cases are called "deviations". They are
>>>>>>>> permissible as long as they are documented with an in-code comment.
>>>>>>>> """
>>>>>>>
>>>>>>> Hmm, so you really mean in-code comments. I don't think this will scale
>>>>>>> well (see e.g. the DCE related intended deviation), and it also goes
>>>>>>> against the "no special casing for every static analysis tool" concern
>>>>>>> I did voice on the call.
>>>>>>
>>>>>> On this subject the idea was more to define a “xen” way to document
>>>>>> deviations in the code and do it in a way so that we could easily 
>>>>>> substitute
>>>>>> the “flag” to adapt it for each analyser using tools or command line 
>>>>>> options.
>>>>>
>>>>> I think the basic scheme of something like this would want laying out
>>>>> before doc changes like the one here actually go in, so that it's clear
>>>>> what the action is if a new deviation needs adding for whatever reason
>>>>> (and also allowing interested people to start contributing patches to
>>>>> add respective annotations).
>>>>
>>>> We will work on that but if we wait for everything to be solved we will
>>>> never progress.
>>>> I have a task on my side (ie at arm) to work on that and Luca Fancellu
>>>> will start working on it next month.
>>>> Now I do not think that this should block this patch, agreeing on rules 
>>>> does
>>>> not mean will respect all of them in the short term so we can wait a bit 
>>>> as I
>>>> definitely think that how to document violations in the code and in general
>>>> will be a work package on its own and will require some discussion.
>>>
>>> Right.
>>>
>>> In general, we'll need to document these deviations and ideally they
>>> would be documented as in-code comments because they are easier to keep
>>> in sync with the code. But we won't be able to do that in all cases.
>>>
>>> We'll also need a special TAG to mark the deviation. Nobody wants
>>> multiple tagging systems for different tools (ECLAIR, cppcheck,
>>> Coverity, etc.) We'll come up with one tagging system and introduce
>>> conversion scripts as needed. Roberto offered to help on the call to
>>> come up with a generic tagging system.
>>>
>>> In some cases in-code comments for every deviation would be too verbose.
>>> We'll want to handle it in another way. It could be a document
>>> somewhere else, or simply disabling the Rules check in ECLAIR/cppcheck
>>> (but that partially defeats the purpose.) We'll have to see. I think
>>> it is going to be on a case by case basis.
>>>
>>>
>>> In short, I don't think we have all the info and expertise to come up
>>> with a good deviation system right now. We need to make more progress
>>> and analize a few specific examples before we can do that. But to gain
>>> that expertise we need to agree on a set of rules we want to follow
>>> first, which is this patch series.
>>>
>>>
>>> So, I think this is the best way we can start the process. We can
>>> clarify further with the comment on top of this file, and we could even
>>> remove the specific part about the "in-code comment" with an open-ended
>>> statement until we come up with a clear deviation strategy. For
>>> instance:
>>>
>>> """
>>> It is possible that in specific circumstances it is best not to follow a
>>> rule because it is not possible or because the alternative leads to
>>> better code quality. Those cases are called "deviations". They are
>>> permissible as long as they are documented.
>>>
>>> The existing codebase is not 100% compliant with the rules. Some of the
>>> violations are meant to be documented as deviations, while some others
>>> should be fixed. Both compliance and documenting deviations on the
>>> existing codebase is work-in-progress.
>>> """
>>
>> This is better, yes, yet I'm still concerned of "existing codebase":
>> Without it being clear how to deal with deviations, what would we do
>> with new additions of deviations? We need to be able to say something
>> concrete in review comments, and prior to getting any review comments
>> people should at least stand a chance of being able to figure out
>> what's expected of them.
> 
> 
> I think you are right that it would be nice to provide a guideline for
> new patches. Even a simple one. For new patches, if it is not an in-code
> comment it could be part of the commit message. (Also it is unlikely
> that a new patch would introduce very many new deviations.)
> 
> What about the following:
> 
> """
> It is possible that in specific circumstances it is best not to follow a
> rule because it is not possible or because the alternative leads to
> better code quality. Those cases are called "deviations". They are
> permissible as long as they are documented, either as an in-code comment
> or as part of the commit message. Other documentation mechanisms are
> work-in-progress.
> 
> The existing codebase is not 100% compliant with the rules. Some of the
> violations are meant to be documented as deviations, while some others
> should be fixed. Both compliance and documenting deviations on the
> existing codebase are work-in-progress.
> """
> 
> The goal is to provide a basic frame of reference for new patches, while
> also saying that we are still working on the documentation system.

Fine with me for the time being.

Jan