| Message ID | 9a27f6cbe552a00274f7ad32eec63f0e80e7644f.1689176790.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN] xen: fix violations of MISRA C:2012 Rule 3.1 | expand |
> On 12 Jul 2023, at 16:54, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote: > > In the file 'xen/common/xmalloc_tlsf.c' is not clear how > the commented-out code should interact with the previous statement. > To resolve the MISRA violation generated by the nested comment > a #if .. #endif block with an explanatory comment substitutes > the earlier construct. > > In the file 'xen/include/xen/atomic.h' the nested comment has been removed, > since the code sample is already explained by the preceding comment. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > Following the suggestion of this message > https://lore.kernel.org/xen-devel/536f3049-41f7-b127-ba94-81925e34ea0f@suse.com/ > an explanatory comment has been added. > --- > xen/common/xmalloc_tlsf.c | 13 ++++++++++--- > xen/include/xen/atomic.h | 2 +- > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c > index c21bf71e88..56c3849414 100644 > --- a/xen/common/xmalloc_tlsf.c > +++ b/xen/common/xmalloc_tlsf.c > @@ -139,10 +139,17 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) > *r = *r + t; > *fl = flsl(*r) - 1; > *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; > - *fl -= FLI_OFFSET; > - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! > - *fl = *sl = 0; > + /* > + * It's unclear what was the purpose of the commented-out code that now > + * is in the #else branch. The current form is motivated by the correction > + * of a violation MISRA:C 2012 Rule 3.1 > */ > +#if 1 > + *fl -= FLI_OFFSET; > +#else > + if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! In the message you linked above, you suggested to use /* FL will be always >0! */, why has it changed? Was some comment I missed? The xen codestyle mandates the use of /* */, anyway I agree that here you are just moving code... So maybe the maintainer can tell what is the best thing to do here. > + *fl = *sl = 0; > +#endif > *r &= ~t; > } > } > diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h > index 529213ebbb..fa750a18ae 100644 > --- a/xen/include/xen/atomic.h > +++ b/xen/include/xen/atomic.h > @@ -78,7 +78,7 @@ static inline void _atomic_set(atomic_t *v, int i); > * int old = atomic_read(&v); > * int new = old + 1; > * if ( likely(old == atomic_cmpxchg(&v, old, new)) ) > - * break; // success! > + * break; > * } > */ > static inline int atomic_cmpxchg(atomic_t *v, int old, int new); > -- > 2.34.1 > > Anyway apart from that, the patch looks ok to me. Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
On 12/07/23 18:02, Luca Fancellu wrote: > > >> On 12 Jul 2023, at 16:54, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote: >> >> In the file 'xen/common/xmalloc_tlsf.c' is not clear how >> the commented-out code should interact with the previous statement. >> To resolve the MISRA violation generated by the nested comment >> a #if .. #endif block with an explanatory comment substitutes >> the earlier construct. >> >> In the file 'xen/include/xen/atomic.h' the nested comment has been removed, >> since the code sample is already explained by the preceding comment. >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> Following the suggestion of this message >> https://lore.kernel.org/xen-devel/536f3049-41f7-b127-ba94-81925e34ea0f@suse.com/ >> an explanatory comment has been added. >> --- >> xen/common/xmalloc_tlsf.c | 13 ++++++++++--- >> xen/include/xen/atomic.h | 2 +- >> 2 files changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c >> index c21bf71e88..56c3849414 100644 >> --- a/xen/common/xmalloc_tlsf.c >> +++ b/xen/common/xmalloc_tlsf.c >> @@ -139,10 +139,17 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) >> *r = *r + t; >> *fl = flsl(*r) - 1; >> *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; >> - *fl -= FLI_OFFSET; >> - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! >> - *fl = *sl = 0; >> + /* >> + * It's unclear what was the purpose of the commented-out code that now >> + * is in the #else branch. The current form is motivated by the correction >> + * of a violation MISRA:C 2012 Rule 3.1 >> */ >> +#if 1 >> + *fl -= FLI_OFFSET; >> +#else >> + if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! > > In the message you linked above, you suggested to use /* FL will be always >0! */, why has it changed? > Was some comment I missed? The xen codestyle mandates the use of /* */, anyway I agree that here you > are just moving code... > So maybe the maintainer can tell what is the best thing to do here. You didn't miss any further comment: my suggestion was related to the explanatory comment, not the nested comment itself. If a better wording can be found for the former, no problem. As for the codestyle point: it does not change anything doing "// FL will be always >0!" -> "/* FL will be always >0! */ w.r.t. Rule 3.1 (both would have been nested comments). Regards,
> On 12 Jul 2023, at 17:17, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote: > > > > On 12/07/23 18:02, Luca Fancellu wrote: >>> On 12 Jul 2023, at 16:54, Nicola Vetrini <nicola.vetrini@bugseng.com> wrote: >>> >>> In the file 'xen/common/xmalloc_tlsf.c' is not clear how >>> the commented-out code should interact with the previous statement. >>> To resolve the MISRA violation generated by the nested comment >>> a #if .. #endif block with an explanatory comment substitutes >>> the earlier construct. >>> >>> In the file 'xen/include/xen/atomic.h' the nested comment has been removed, >>> since the code sample is already explained by the preceding comment. >>> >>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>> --- >>> Following the suggestion of this message >>> https://lore.kernel.org/xen-devel/536f3049-41f7-b127-ba94-81925e34ea0f@suse.com/ >>> an explanatory comment has been added. >>> --- >>> xen/common/xmalloc_tlsf.c | 13 ++++++++++--- >>> xen/include/xen/atomic.h | 2 +- >>> 2 files changed, 11 insertions(+), 4 deletions(-) >>> >>> diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c >>> index c21bf71e88..56c3849414 100644 >>> --- a/xen/common/xmalloc_tlsf.c >>> +++ b/xen/common/xmalloc_tlsf.c >>> @@ -139,10 +139,17 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) >>> *r = *r + t; >>> *fl = flsl(*r) - 1; >>> *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; >>> - *fl -= FLI_OFFSET; >>> - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! >>> - *fl = *sl = 0; >>> + /* >>> + * It's unclear what was the purpose of the commented-out code that now >>> + * is in the #else branch. The current form is motivated by the correction >>> + * of a violation MISRA:C 2012 Rule 3.1 >>> */ >>> +#if 1 >>> + *fl -= FLI_OFFSET; >>> +#else >>> + if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! >> In the message you linked above, you suggested to use /* FL will be always >0! */, why has it changed? >> Was some comment I missed? The xen codestyle mandates the use of /* */, anyway I agree that here you >> are just moving code... >> So maybe the maintainer can tell what is the best thing to do here. > > You didn't miss any further comment: my suggestion was related to the explanatory comment, not the nested comment itself. If a better wording can be found for the former, no problem. As for the codestyle point: it does not change anything doing > "// FL will be always >0!" -> "/* FL will be always >0! */ > w.r.t. Rule 3.1 (both would have been nested comments). Yes, I agree it does not change anything, now that I read better the message, it is from Jan suggesting this: #if 1 *fl -= FLI_OFFSET; #else if ((*fl -= FLI_OFFSET) < 0) /* FL will be always >0! */ *fl = *sl = 0; #endif So using /* FL will be always >0! */ instead of copying // FL will be always >0! Anyway, I think it can be addressed on commit, whatever form the maintainer prefers. > > Regards, > > -- > Nicola Vetrini, BSc > Software Engineer, BUGSENG srl (https://bugseng.com)
On Wed, 12 Jul 2023, Nicola Vetrini wrote: > In the file 'xen/common/xmalloc_tlsf.c' is not clear how > the commented-out code should interact with the previous statement. > To resolve the MISRA violation generated by the nested comment > a #if .. #endif block with an explanatory comment substitutes > the earlier construct. > > In the file 'xen/include/xen/atomic.h' the nested comment has been removed, > since the code sample is already explained by the preceding comment. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > Following the suggestion of this message > https://lore.kernel.org/xen-devel/536f3049-41f7-b127-ba94-81925e34ea0f@suse.com/ > an explanatory comment has been added. > --- > xen/common/xmalloc_tlsf.c | 13 ++++++++++--- > xen/include/xen/atomic.h | 2 +- > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c > index c21bf71e88..56c3849414 100644 > --- a/xen/common/xmalloc_tlsf.c > +++ b/xen/common/xmalloc_tlsf.c > @@ -139,10 +139,17 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) > *r = *r + t; > *fl = flsl(*r) - 1; > *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; > - *fl -= FLI_OFFSET; > - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! > - *fl = *sl = 0; > + /* > + * It's unclear what was the purpose of the commented-out code that now > + * is in the #else branch. The current form is motivated by the correction > + * of a violation MISRA:C 2012 Rule 3.1 > */ > +#if 1 > + *fl -= FLI_OFFSET; > +#else > + if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! > + *fl = *sl = 0; > +#endif > *r &= ~t; > } > } > diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h > index 529213ebbb..fa750a18ae 100644 > --- a/xen/include/xen/atomic.h > +++ b/xen/include/xen/atomic.h > @@ -78,7 +78,7 @@ static inline void _atomic_set(atomic_t *v, int i); > * int old = atomic_read(&v); > * int new = old + 1; > * if ( likely(old == atomic_cmpxchg(&v, old, new)) ) > - * break; // success! > + * break; > * } > */ > static inline int atomic_cmpxchg(atomic_t *v, int old, int new); > -- > 2.34.1 >
diff --git a/xen/common/xmalloc_tlsf.c b/xen/common/xmalloc_tlsf.c index c21bf71e88..56c3849414 100644 --- a/xen/common/xmalloc_tlsf.c +++ b/xen/common/xmalloc_tlsf.c @@ -139,10 +139,17 @@ static inline void MAPPING_SEARCH(unsigned long *r, int *fl, int *sl) *r = *r + t; *fl = flsl(*r) - 1; *sl = (*r >> (*fl - MAX_LOG2_SLI)) - MAX_SLI; - *fl -= FLI_OFFSET; - /*if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! - *fl = *sl = 0; + /* + * It's unclear what was the purpose of the commented-out code that now + * is in the #else branch. The current form is motivated by the correction + * of a violation MISRA:C 2012 Rule 3.1 */ +#if 1 + *fl -= FLI_OFFSET; +#else + if ((*fl -= FLI_OFFSET) < 0) // FL will be always >0! + *fl = *sl = 0; +#endif *r &= ~t; } } diff --git a/xen/include/xen/atomic.h b/xen/include/xen/atomic.h index 529213ebbb..fa750a18ae 100644 --- a/xen/include/xen/atomic.h +++ b/xen/include/xen/atomic.h @@ -78,7 +78,7 @@ static inline void _atomic_set(atomic_t *v, int i); * int old = atomic_read(&v); * int new = old + 1; * if ( likely(old == atomic_cmpxchg(&v, old, new)) ) - * break; // success! + * break; * } */ static inline int atomic_cmpxchg(atomic_t *v, int old, int new);
In the file 'xen/common/xmalloc_tlsf.c' is not clear how the commented-out code should interact with the previous statement. To resolve the MISRA violation generated by the nested comment a #if .. #endif block with an explanatory comment substitutes the earlier construct. In the file 'xen/include/xen/atomic.h' the nested comment has been removed, since the code sample is already explained by the preceding comment. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- Following the suggestion of this message https://lore.kernel.org/xen-devel/536f3049-41f7-b127-ba94-81925e34ea0f@suse.com/ an explanatory comment has been added. --- xen/common/xmalloc_tlsf.c | 13 ++++++++++--- xen/include/xen/atomic.h | 2 +- 2 files changed, 11 insertions(+), 4 deletions(-)