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

[Thomas Huth]

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 <address@hidden>
> ---
>  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++) {
> -                if (bptr[ix] == '/')
> +                if (bptr[ix] == '/') {
>                      nbslash++;
> +                }
>              }
>          }
>          len = strlen(uptr) + 1;
>      }
>  
>      if (nbslash == 0) {
> -        if (uptr != NULL)
> +        if (uptr != NULL) {
>              /* exception characters from uri_to_string */
> -            val = uri_string_escape(uptr, "/;&=+$,");
> +        }
> +        val = uri_string_escape(uptr, "/;&=+$,");

That's a bug: uri_string_escape() should be within the curly braces instead.

By the way, found that one with the following trick: Compare the disassemblies
before and after you're changes:

 git checkout master
 make util/uri.o
 strip util/uri.o
 objdump -Drx util/uri.o > /tmp/uri-master.txt
 git checkout cleanupbranch
 make util/uri.o
 strip util/uri.o
 objdump -Drx util/uri.o > /tmp/uri-cleanup.txt
 diff -u /tmp/uri-*.txt

Since you're only doing coding style clean-up, there should not be
any diff in the resulting assembler code.

 Cheers,
  Thomas