[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





--- Comment #4 from anton muhin <antonm at chromium.org>  2010-01-27 08:53:26 PST ---
(In reply to comment #2)
> (From update of attachment 47426 [details])
> Thanks for this fix! Some nitpicks:
> 
> > +def up_to_matching_paren(s):
> 
> Please add a docstring for this function.

Done.

> 
> > +    # 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)' ?

Good catch.  Fixed.

> > +        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.

For sure.  I prefer named groups too, but didn't think it's a common style.

> > +                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.

Done.

> 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).

Added some more tests.  Please, let me know if it's not enough.

> 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.

How can I do it?  Running against full source tree seems kind of useless---too
difficult to say noise from real problems?  Is there some other approach?

E.g. it might be convenient to have 'best of the breed' sample of sources to
check modified linter against.

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