Re: [Qemu-devel] [PATCH v3 3/3] util/uri.c: add brackets to `if` statements

[苏航]

Hi, when I try to change 
`while (cond) ;` 
to
`while (cond) {
}`
checkpatch.pl complains about this:
'''
ERROR: suspect code indent for conditional statements (8, 8)
#1506: FILE: uri.c:1506:
+while ((*tmp++ = *segp++) != 0) {
[...]
+}

ERROR: suspect code indent for conditional statements (8, 8)
#1512: FILE: uri.c:1512:
+while ((segp > path) && ((--segp)[0] == '/')) {
[...]
+}
'''

When I add a semicolon, checkpatch.pl stop complaining.
`while (cond) {
;
}`
What should I do now?

"Eric Blake" wrote:
> On 02/23/2018 03:34 AM, Thomas Huth wrote:
> > On 23.02.2018 08:51, Su Hang wrote:
> >> Add brackets that wrap `if`, `else`, `while` that hold single
> >> statements.
> >>
> >> In order to do this, I write a simple python regex script.
> 
> Without documenting the script, no one else can reproduce this; but it's 
> no different than if they had manually made changes instead of trying to 
> script it, so I'm not sure this sentence adds much in its current form.
> 
> >>
> >> Since then, all complaints rised by checkpatch.pl has been suppressed.
> 
> s/rised/raised/
> s/Since then,/With that/
> s/has/have/
> 
> >>
> >> Signed-off-by: Su Hang 
> >> ---
> >>   util/uri.c | 462 
> >> ++---
> >>   1 file changed, 291 insertions(+), 171 deletions(-)
> >>
> 
> >>   cur = *str;
> >> -if (!ISA_ALPHA(cur))
> >> +if (!ISA_ALPHA(cur)) {
> >>   return 2;
> >> +}
> >>   cur++;
> >> -while (ISA_ALPHA(cur) || ISA_DIGIT(cur) ||
> >> -   (*cur == '+') || (*cur == '-') || (*cur == '.'))
> >> +while (ISA_ALPHA(cur) || ISA_DIGIT(cur) || (*cur == '+') || (*cur == 
> >> '-') ||
> >> +   (*cur == '.'))
> >>   cur++;
> > 
> > You've changed the while statement, but checkpatch.pl apparently does not
> > complain about missing curly braces here ... that's strange, I thought we'd
> > also wanted to enforce curly braces for while loops.
> 
> Maybe because it gets lost since the condition expanded over more than 
> one line?  But yes, now that we've noticed it manually, it should be 
> fixed.  While at it, you can avoid the redundant ():
> 
> while (ISA_ALPHA(cur) || ISA_DIGIT(cur) || *cur == '+' || *cur == '-' ||
> *cur == '.') {
> 
> 
> >> -while ((*tmp++ = *segp++) != 0)
> >> +while ((*tmp++ = *segp++) != 0) {
> >>   ;
> >> +}
> > 
> > A bikeshed-painting-friday question for everybody on qemu-devel:
> > Should there be a single semicolon inside curly braces in this case, or not?
> > 
> 
> Checkpatch doesn't complain, but lone ';' statements are rare.  I'd omit 
> it, and use just:
> 
> while (cond) {
> }
> 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3 3/3] util/uri.c: add brackets to `if` statements

[苏航]

Thanks for your comments :)
I will pay more attention to what you point out.

"Eric Blake" wrote:
> On 02/23/2018 03:34 AM, Thomas Huth wrote:
> > On 23.02.2018 08:51, Su Hang wrote:
> >> Add brackets that wrap `if`, `else`, `while` that hold single
> >> statements.
> >>
> >> In order to do this, I write a simple python regex script.
> 
> Without documenting the script, no one else can reproduce this; but it's 
> no different than if they had manually made changes instead of trying to 
> script it, so I'm not sure this sentence adds much in its current form.
> 
> >>
> >> Since then, all complaints rised by checkpatch.pl has been suppressed.
> 
> s/rised/raised/
> s/Since then,/With that/
> s/has/have/
> 
> >>
> >> Signed-off-by: Su Hang 
> >> ---
> >>   util/uri.c | 462 
> >> ++---
> >>   1 file changed, 291 insertions(+), 171 deletions(-)
> >>
> 
> >>   cur = *str;
> >> -if (!ISA_ALPHA(cur))
> >> +if (!ISA_ALPHA(cur)) {
> >>   return 2;
> >> +}
> >>   cur++;
> >> -while (ISA_ALPHA(cur) || ISA_DIGIT(cur) ||
> >> -   (*cur == '+') || (*cur == '-') || (*cur == '.'))
> >> +while (ISA_ALPHA(cur) || ISA_DIGIT(cur) || (*cur == '+') || (*cur == 
> >> '-') ||
> >> +   (*cur == '.'))
> >>   cur++;
> > 
> > You've changed the while statement, but checkpatch.pl apparently does not
> > complain about missing curly braces here ... that's strange, I thought we'd
> > also wanted to enforce curly braces for while loops.
> 
> Maybe because it gets lost since the condition expanded over more than 
> one line?  But yes, now that we've noticed it manually, it should be 
> fixed.  While at it, you can avoid the redundant ():
> 
> while (ISA_ALPHA(cur) || ISA_DIGIT(cur) || *cur == '+' || *cur == '-' ||
> *cur == '.') {
> 
> 
> >> -while ((*tmp++ = *segp++) != 0)
> >> +while ((*tmp++ = *segp++) != 0) {
> >>   ;
> >> +}
> > 
> > A bikeshed-painting-friday question for everybody on qemu-devel:
> > Should there be a single semicolon inside curly braces in this case, or not?
> > 
> 
> Checkpatch doesn't complain, but lone ';' statements are rare.  I'd omit 
> it, and use just:
> 
> while (cond) {
> }
> 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3 3/3] util/uri.c: add brackets to `if` statements

[苏航]

Thanks for your reply. ^_^
I will apply all your suggestion in my next patch.

> -Original Messages-
> From: "Thomas Huth" 
> Sent Time: 2018-02-23 17:34:12 (Friday)
> To: "Su Hang" , stefa...@redhat.com
> Cc: qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH v3 3/3] util/uri.c: add brackets to `if` 
> statements
> 
> On 23.02.2018 08:51, Su Hang wrote:
> > Add brackets that wrap `if`, `else`, `while` that hold single
> > statements.
> > 
> > In order to do this, I write a simple python regex script.
> > 
> > Since then, all complaints rised by checkpatch.pl has been suppressed.
> > 
> > Signed-off-by: Su Hang 
> > ---
> >  util/uri.c | 462 
> > ++---
> >  1 file changed, 291 insertions(+), 171 deletions(-)
> > 
> > diff --git a/util/uri.c b/util/uri.c
> > index 278e876ab8b1..48f7298787b1 100644
> > --- a/util/uri.c
> > +++ b/util/uri.c
> > @@ -105,18 +105,18 @@ static void uri_clean(URI *uri);
> >   */
> >  
> >  #define IS_UNWISE(p)   
> > \
> > -  (((*(p) == '{')) || ((*(p) == '}')) || ((*(p) == '|')) ||
> > \
> > -   ((*(p) == '\\')) || ((*(p) == '^')) || ((*(p) == '[')) ||   
> > \
> > -   ((*(p) == ']')) || ((*(p) == '`')))
> > +(((*(p) == '{')) || ((*(p) == '}')) || ((*(p) == '|')) ||  
> > \
> > + ((*(p) == '\\')) || ((*(p) == '^')) || ((*(p) == '[')) || 
> > \
> > + ((*(p) == ']')) || ((*(p) == '`')))
> >  /*
> >   * reserved = ";" | "/" | "?" | ":" | "@" | "&" | "=" | "+" | "$" | "," |
> >   *"[" | "]"
> >   */
> >  
> >  #define IS_RESERVED(x) (((x) == ';') || ((x) == '/') || ((x) == '?') ||
> > \
> > -((x) == ':') || ((x) == '@') || ((x) == '&') || ((x) == '=') ||
> > \
> > -((x) == '+') || ((x) == '$') || ((x) == ',') || ((x) == '[') ||
> > \
> > -((x) == ']'))
> > +  ((x) == ':') || ((x) == '@') || ((x) == '&') || ((x) == '=') ||  
> > \
> > +  ((x) == '+') || ((x) == '$') || ((x) == ',') || ((x) == '[') ||  
> > \
> > +  ((x) == ']'))
> 
> The above whitespace changes should ideally be done in the first patch 
> instead.
> 
> >  /*
> >   * unreserved = alphanum | mark
> > @@ -211,15 +211,17 @@ static int rfc3986_parse_scheme(URI *uri, const char 
> > **str)
> >  {
> >  const char *cur;
> >  
> > -if (str == NULL)
> > +if (str == NULL) {
> >  return -1;
> > +}
> >  
> >  cur = *str;
> > -if (!ISA_ALPHA(cur))
> > +if (!ISA_ALPHA(cur)) {
> >  return 2;
> > +}
> >  cur++;
> > -while (ISA_ALPHA(cur) || ISA_DIGIT(cur) ||
> > -   (*cur == '+') || (*cur == '-') || (*cur == '.'))
> > +while (ISA_ALPHA(cur) || ISA_DIGIT(cur) || (*cur == '+') || (*cur == 
> > '-') ||
> > +   (*cur == '.'))
> >  cur++;
> 
> You've changed the while statement, but checkpatch.pl apparently does not
> complain about missing curly braces here ... that's strange, I thought we'd
> also wanted to enforce curly braces for while loops. Anyway, could you please
> add curly braces around the "*cur++;" here, too?
> 
> > @@ -1437,15 +1503,18 @@ done_cd:
> >  /* string will overlap, do not use strcpy */
> >  tmp = cur;
> >  segp += 3;
> > -while ((*tmp++ = *segp++) != 0)
> > +while ((*tmp++ = *segp++) != 0) {
> >  ;
> > +}
> 
> A bikeshed-painting-friday question for everybody on qemu-devel:
> Should there be a single semicolon inside curly braces in this case, or not?
> 
> >  /* If there are no previous segments, then keep going from here.  
> > */
> >  segp = cur;
> > -while ((segp > path) && ((--segp)[0] == '/'))
> > +while ((segp > path) && ((--segp)[0] == '/')) {
> >  ;
> 
> (dito)
> 
> > -if (segp == path)
> > +}
> > +if (segp == path) {
> >  continue;
> > +}
> >  
> >  /* "segp" is pointing to the end of a previous segment; find it's
> >   * start.  We need to back up to the previous segment and start
> [...]
> > @@ -1491,8 +1562,9 @@ done_cd:
> >  static int is_hex(char c)
> >  {
> >  if (((c >= '0') && (c <= '9')) || ((c >= 'a') && (c <= 'f')) ||
> > -((c >= 'A') && (c <= 'F')))
> > +((c >= 'A') && (c <= 'F'))) {
> >  return 1;
> > +}
> >  return 0;
> >  }
> 
> Not related to your patch, but an idea for a future clean-up patch:
> We've already got qemu_isxdigit(), so there is no real need for this
> separate is_hex() function.
> 
> [...]
> > @@ -2020,17 +2127,19 @@ char *uri_resolve_relative(const char *uri, const 
> > char *base)
> >   */
> >  if (bptr[pos] != ref->path[pos]) { /* check for trivial URI == 
> > base */
> >  for (; bptr[ix] != 0; ix++) {