| Message ID | 9fbc2bcfa1ee019a8ac1cd1a3d29c38b59b8edff.1699295113.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | address MISRA C:2012 Rule 15.2 | expand |
On 07/11/2023 10:33 am, Nicola Vetrini wrote: > The backwards goto in the vsnprintf function can be replaced > with a loop, thereby fixing a violation of MISRA C:2012 Rule 15.2. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > xen/common/vsprintf.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c > index c49631c0a4d8..603bae44177a 100644 > --- a/xen/common/vsprintf.c > +++ b/xen/common/vsprintf.c > @@ -495,6 +495,8 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args) > } > > for (; *fmt ; ++fmt) { > + bool repeat = true; > + > if (*fmt != '%') { > if (str < end) > *str = *fmt; > @@ -504,14 +506,16 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args) > > /* process flags */ > flags = 0; > - repeat: > - ++fmt; /* this also skips first '%' */ > - switch (*fmt) { > - case '-': flags |= LEFT; goto repeat; > - case '+': flags |= PLUS; goto repeat; > - case ' ': flags |= SPACE; goto repeat; > - case '#': flags |= SPECIAL; goto repeat; > - case '0': flags |= ZEROPAD; goto repeat; > + while ( repeat ) { > + ++fmt; /* this also skips the first '%' */ > + switch (*fmt) { > + case '-': flags |= LEFT; break; > + case '+': flags |= PLUS; break; > + case ' ': flags |= SPACE; break; > + case '#': flags |= SPECIAL; break; > + case '0': flags |= ZEROPAD; break; > + default: repeat = false; break; > + } I'm firmly against this change. It takes a simple and clear piece of code and replaces it with something harder to follow because you have to look elsewhere to figure how the variable works. Labels with names such as repeat/again/retry are clearly forming a loop(ish). I see in patch 4 that you exempt again/retry. That list needs to include repeat, and this patch wants dropping. ~Andrew
On 07.11.2023 12:36, Andrew Cooper wrote: > On 07/11/2023 10:33 am, Nicola Vetrini wrote: >> The backwards goto in the vsnprintf function can be replaced >> with a loop, thereby fixing a violation of MISRA C:2012 Rule 15.2. >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> xen/common/vsprintf.c | 20 ++++++++++++-------- >> 1 file changed, 12 insertions(+), 8 deletions(-) >> >> diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c >> index c49631c0a4d8..603bae44177a 100644 >> --- a/xen/common/vsprintf.c >> +++ b/xen/common/vsprintf.c >> @@ -495,6 +495,8 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args) >> } >> >> for (; *fmt ; ++fmt) { >> + bool repeat = true; >> + >> if (*fmt != '%') { >> if (str < end) >> *str = *fmt; >> @@ -504,14 +506,16 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args) >> >> /* process flags */ >> flags = 0; >> - repeat: >> - ++fmt; /* this also skips first '%' */ >> - switch (*fmt) { >> - case '-': flags |= LEFT; goto repeat; >> - case '+': flags |= PLUS; goto repeat; >> - case ' ': flags |= SPACE; goto repeat; >> - case '#': flags |= SPECIAL; goto repeat; >> - case '0': flags |= ZEROPAD; goto repeat; >> + while ( repeat ) { >> + ++fmt; /* this also skips the first '%' */ >> + switch (*fmt) { >> + case '-': flags |= LEFT; break; >> + case '+': flags |= PLUS; break; >> + case ' ': flags |= SPACE; break; >> + case '#': flags |= SPECIAL; break; >> + case '0': flags |= ZEROPAD; break; >> + default: repeat = false; break; >> + } > > I'm firmly against this change. It takes a simple and clear piece of > code and replaces it with something harder to follow because you have to > look elsewhere to figure how the variable works. While I don't really like that change either, I also don't like uses of goto (at some point we said using it for error handling is okay, but the case here is clearly not in that category). So at least for consideration, how about getting away without a new variable: for ( ; ; ) { ++fmt; /* this also skips the first '%' */ switch ( *fmt ) { case '-': flags |= LEFT; continue; case '+': flags |= PLUS; continue; case ' ': flags |= SPACE; continue; case '#': flags |= SPECIAL; continue; case '0': flags |= ZEROPAD; continue; } break; } Jan
diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c index c49631c0a4d8..603bae44177a 100644 --- a/xen/common/vsprintf.c +++ b/xen/common/vsprintf.c @@ -495,6 +495,8 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args) } for (; *fmt ; ++fmt) { + bool repeat = true; + if (*fmt != '%') { if (str < end) *str = *fmt; @@ -504,14 +506,16 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args) /* process flags */ flags = 0; - repeat: - ++fmt; /* this also skips first '%' */ - switch (*fmt) { - case '-': flags |= LEFT; goto repeat; - case '+': flags |= PLUS; goto repeat; - case ' ': flags |= SPACE; goto repeat; - case '#': flags |= SPECIAL; goto repeat; - case '0': flags |= ZEROPAD; goto repeat; + while ( repeat ) { + ++fmt; /* this also skips the first '%' */ + switch (*fmt) { + case '-': flags |= LEFT; break; + case '+': flags |= PLUS; break; + case ' ': flags |= SPACE; break; + case '#': flags |= SPECIAL; break; + case '0': flags |= ZEROPAD; break; + default: repeat = false; break; + } } /* get field width */
The backwards goto in the vsnprintf function can be replaced with a loop, thereby fixing a violation of MISRA C:2012 Rule 15.2. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/common/vsprintf.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)