[Webkit-unassigned] [Bug 34173] style tool: Improve treatment of conditions and rest of the line for if, else, switch and alikes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=34173


Shinichiro Hamaji <hamaji at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #47426|review?                     |review-
               Flag|                            |




--- Comment #2 from Shinichiro Hamaji <hamaji at chromium.org>  2010-01-27 00:45:40 PST ---
(From update of attachment 47426)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.