Thanks for your reply. ^_^
I will apply all your suggestion in my next patch.
> -----Original Messages-----
> From: "Thomas Huth" <th...@redhat.com>
> Sent Time: 2018-02-23 17:34:12 (Friday)
> To: "Su Hang" <suhan...@mails.ucas.ac.cn>, 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 <suhan...@mails.ucas.ac.cn>
> > ---
> > 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
