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 <[email protected]> wrote: >>>>> On 26.05.2022 11:54, Bertrand Marquis wrote: >>>>>>> On 26 May 2022, at 10:43, Jan Beulich <[email protected]> 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
