[webkit-reviews] review denied: [Bug 34173] style tool: Improve treatment of conditions and rest of the line for if, else, switch and alikes : [Attachment 47426] First take

[bugzilla-daemon at webkit.org]

Shinichiro Hamaji <hamaji at chromium.org> has denied anton muhin
<antonm at chromium.org>'s request for review:
Bug 34173: style tool: Improve treatment of conditions and rest of the line for
if, else, switch and alikes
https://bugs.webkit.org/show_bug.cgi?id=34173

Attachment 47426: First take
https://bugs.webkit.org/attachment.cgi?id=47426&action=review

------- Additional Comments from Shinichiro Hamaji <hamaji at chromium.org>
Thanks for this fix! Some nitpicks:

> +def up_to_matching_paren(s):

Please add a docstring for this function.

> +    # And rules for macro definitions are relaxed.
> +    if not match(r'\s*#define', line):

I think we still want to complain about '#define if ( wrong_spacing)' ?

> +	   matched = search(r'\b(if|for|foreach|while|switch)\s*\((.*)$', line)


The code was more difficult to read before your change, thanks. However, could
you use '(?P<statement>if|for|foreach|while|switch)' and
matched.group('steatement') instead of '(if|for|foreach|while|switch)' and
matched.group(1) ? We are using this style in new code. The same comment would
apply for other matched.group()s, too.

> +		   if not match(r'(\s*{\s*}?$)|(\s*;?$)', rest):
> +		       error(line_number, 'whitespace/parens', 4,
> +			     'More than one command on the same line')

I think it would be better if the error message contains the value of
statement.

Could you add more test cases? We may want to check there are no alarms for 'if
((a & b)' (so we can check up_to_matching_paren explicitly).

Also, if you haven't done yet, please run your modified style checker for a
bunch of code and check if there are no false alarms.