diff mbox series

[v2] docs/misra/rules.rst: add Rule 17.1

Message ID alpine.DEB.2.22.394.2312081738100.1703076@ubuntu-linux-20-04-desktop (mailing list archive)
State New
Headers show
Series [v2] docs/misra/rules.rst: add Rule 17.1 | expand

Commit Message

Stefano Stabellini Dec. 9, 2023, 1:39 a.m. UTC
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
Changes in v2:
- separated 17.1 in its own patch
- add a comment

Comments

Julien Grall Dec. 15, 2023, 10:13 a.m. UTC | #1
Hi Stefano,

On 09/12/2023 01:39, Stefano Stabellini wrote:
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> ---
> Changes in v2:
> - separated 17.1 in its own patch
> - add a comment
> 
> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> index 8a659d8d47..f29b4c3d9a 100644
> --- a/docs/misra/rules.rst
> +++ b/docs/misra/rules.rst
> @@ -189,6 +189,12 @@ existing codebase are work-in-progress.
>        - A switch-expression shall not have essentially Boolean type
>        -
>   
> +   * - `Rule 17.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_17_01.c>`_
> +     - Required
> +     - The features of <stdarg.h> shall not be used
> +     - It is understood that in some limited circumstances <stdarg.h> is
> +       appropriate to use, such as the implementation of printk.

The last bullet point is unclear to me. You don't define what 
"appropriate" means here. So who is going to decide? Also, how is this 
going to be deviated?

Possibly the solution here is to remove the last bullet point and have a 
paragraph in deviations.rst explaining why we are using va_args.

Cheers,
Stefano Stabellini Dec. 15, 2023, 9:02 p.m. UTC | #2
On Fri, 15 Dec 2023, Julien Grall wrote:
> Hi Stefano,
> 
> On 09/12/2023 01:39, Stefano Stabellini wrote:
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> > ---
> > Changes in v2:
> > - separated 17.1 in its own patch
> > - add a comment
> > 
> > diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> > index 8a659d8d47..f29b4c3d9a 100644
> > --- a/docs/misra/rules.rst
> > +++ b/docs/misra/rules.rst
> > @@ -189,6 +189,12 @@ existing codebase are work-in-progress.
> >        - A switch-expression shall not have essentially Boolean type
> >        -
> >   +   * - `Rule 17.1
> > <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_17_01.c>`_
> > +     - Required
> > +     - The features of <stdarg.h> shall not be used
> > +     - It is understood that in some limited circumstances <stdarg.h> is
> > +       appropriate to use, such as the implementation of printk.
> 
> The last bullet point is unclear to me. You don't define what "appropriate"
> means here. So who is going to decide? Also, how is this going to be deviated?
> 
> Possibly the solution here is to remove the last bullet point and have a
> paragraph in deviations.rst explaining why we are using va_args.

Actually, I agree with you. I added the last bullet to address Jan's
concern:
https://marc.info/?l=xen-devel&m=170191695511513
https://marc.info/?l=xen-devel&m=170193528120968

This was my original reply:

"We agreed that in certain situations stdarg.h is OK to use and in those
cases we would add a deviation. Would you like me to add something to
that effect here? I could do that but it would sound a bit vague.  Also
if we want to specify a project-wide deviation it would be better
documented in docs/misra/deviations.rst. I would leave Rule 17.1 without
a note."

My preference is still to remove the last bullet (because too generic)
and add any specific information to deviations.rst as usual.

Julien, would you be OK with this patch if I remove the last bullet and
leave it blank?
Julien Grall Dec. 19, 2023, 6:39 p.m. UTC | #3
Hi Stefano,

On 15/12/2023 21:02, Stefano Stabellini wrote:
> On Fri, 15 Dec 2023, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 09/12/2023 01:39, Stefano Stabellini wrote:
>>>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
>>> ---
>>> Changes in v2:
>>> - separated 17.1 in its own patch
>>> - add a comment
>>>
>>> diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
>>> index 8a659d8d47..f29b4c3d9a 100644
>>> --- a/docs/misra/rules.rst
>>> +++ b/docs/misra/rules.rst
>>> @@ -189,6 +189,12 @@ existing codebase are work-in-progress.
>>>         - A switch-expression shall not have essentially Boolean type
>>>         -
>>>    +   * - `Rule 17.1
>>> <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_17_01.c>`_
>>> +     - Required
>>> +     - The features of <stdarg.h> shall not be used
>>> +     - It is understood that in some limited circumstances <stdarg.h> is
>>> +       appropriate to use, such as the implementation of printk.
>>
>> The last bullet point is unclear to me. You don't define what "appropriate"
>> means here. So who is going to decide? Also, how is this going to be deviated?
>>
>> Possibly the solution here is to remove the last bullet point and have a
>> paragraph in deviations.rst explaining why we are using va_args.
> 
> Actually, I agree with you. I added the last bullet to address Jan's
> concern:
> https://marc.info/?l=xen-devel&m=170191695511513
> https://marc.info/?l=xen-devel&m=170193528120968
> 
> This was my original reply:
> 
> "We agreed that in certain situations stdarg.h is OK to use and in those
> cases we would add a deviation. Would you like me to add something to
> that effect here? I could do that but it would sound a bit vague.  Also
> if we want to specify a project-wide deviation it would be better
> documented in docs/misra/deviations.rst. I would leave Rule 17.1 without
> a note."
> 
> My preference is still to remove the last bullet (because too generic)
> and add any specific information to deviations.rst as usual.
> 
> Julien, would you be OK with this patch if I remove the last bullet and
> leave it blank?

I would be fine with that:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,
Stefano Stabellini Dec. 20, 2023, 12:34 a.m. UTC | #4
On Tue, 19 Dec 2023, Julien Grall wrote:
> Hi Stefano,
> 
> On 15/12/2023 21:02, Stefano Stabellini wrote:
> > On Fri, 15 Dec 2023, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 09/12/2023 01:39, Stefano Stabellini wrote:
> > > > 
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
> > > > ---
> > > > Changes in v2:
> > > > - separated 17.1 in its own patch
> > > > - add a comment
> > > > 
> > > > diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
> > > > index 8a659d8d47..f29b4c3d9a 100644
> > > > --- a/docs/misra/rules.rst
> > > > +++ b/docs/misra/rules.rst
> > > > @@ -189,6 +189,12 @@ existing codebase are work-in-progress.
> > > >         - A switch-expression shall not have essentially Boolean type
> > > >         -
> > > >    +   * - `Rule 17.1
> > > > <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_17_01.c>`_
> > > > +     - Required
> > > > +     - The features of <stdarg.h> shall not be used
> > > > +     - It is understood that in some limited circumstances <stdarg.h>
> > > > is
> > > > +       appropriate to use, such as the implementation of printk.
> > > 
> > > The last bullet point is unclear to me. You don't define what
> > > "appropriate"
> > > means here. So who is going to decide? Also, how is this going to be
> > > deviated?
> > > 
> > > Possibly the solution here is to remove the last bullet point and have a
> > > paragraph in deviations.rst explaining why we are using va_args.
> > 
> > Actually, I agree with you. I added the last bullet to address Jan's
> > concern:
> > https://marc.info/?l=xen-devel&m=170191695511513
> > https://marc.info/?l=xen-devel&m=170193528120968
> > 
> > This was my original reply:
> > 
> > "We agreed that in certain situations stdarg.h is OK to use and in those
> > cases we would add a deviation. Would you like me to add something to
> > that effect here? I could do that but it would sound a bit vague.  Also
> > if we want to specify a project-wide deviation it would be better
> > documented in docs/misra/deviations.rst. I would leave Rule 17.1 without
> > a note."
> > 
> > My preference is still to remove the last bullet (because too generic)
> > and add any specific information to deviations.rst as usual.
> > 
> > Julien, would you be OK with this patch if I remove the last bullet and
> > leave it blank?
> 
> I would be fine with that:
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

I plan to commit that one of the next few days unless someone speaks up
diff mbox series

Patch

diff --git a/docs/misra/rules.rst b/docs/misra/rules.rst
index 8a659d8d47..f29b4c3d9a 100644
--- a/docs/misra/rules.rst
+++ b/docs/misra/rules.rst
@@ -189,6 +189,12 @@  existing codebase are work-in-progress.
      - A switch-expression shall not have essentially Boolean type
      -
 
+   * - `Rule 17.1 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_17_01.c>`_
+     - Required
+     - The features of <stdarg.h> shall not be used
+     - It is understood that in some limited circumstances <stdarg.h> is
+       appropriate to use, such as the implementation of printk.
+
    * - `Rule 17.3 <https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_17_03.c>`_
      - Mandatory
      - A function shall not be declared implicitly