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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.