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