Bug 34173 – style tool: Improve treatment of conditions and rest of the line for if, else, switch and alikes

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[anton muhin]

Improve treatment of conditions and rest of the line for if, else, switch and alikes

[anton muhin]

Created attachment 47426 [details]
First take

[Shinichiro Hamaji]

Comment on attachment 47426 [details]
First take

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.

[anton muhin]

Created attachment 47541 [details]
Patch

[anton muhin]

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

[Shinichiro Hamaji]

Comment on attachment 47541 [details]
Patch

Thanks for fixes! Please feel free to land this patch if you agree with my comments and fix them.

> +def up_to_unmatched_closing_paren(s):
> +    """Splits a string into two parts up to first unmatched ) (assuming
> +    that |s| is a rest right after (.
> +
> +    Returns None, None if there is no unmatched )
> +    """

Please follow conventions of docstrings. It should be like

"""Splits a string into two parts up to first unmatched ')'.

Args:
  s: a string which is a substring of line after "(".
     (e.g., "a == (b + c)) {").

Returns:
  A pair of strings (e.g., "a == (b + c))" and " {").

"""

or something like this. It would be nicer if the argument s has a better name, I couldn't come up with one though.

> +    in_macro = match(r'\s*#define', line)

I think we can move this line to just before the check of single line statements (line 1350).

[anton muhin]

Created attachment 47544 [details]
Next round

[anton muhin]

(In reply to comment #5)
> (From update of attachment 47541 [details])
> Thanks for fixes! Please feel free to land this patch if you agree with my
> comments and fix them.
> 
> > +def up_to_unmatched_closing_paren(s):
> > +    """Splits a string into two parts up to first unmatched ) (assuming
> > +    that |s| is a rest right after (.
> > +
> > +    Returns None, None if there is no unmatched )
> > +    """
> 
> Please follow conventions of docstrings. It should be like
> 
> """Splits a string into two parts up to first unmatched ')'.
> 
> Args:
>   s: a string which is a substring of line after "(".
>      (e.g., "a == (b + c)) {").
> 
> Returns:
>   A pair of strings (e.g., "a == (b + c))" and " {").
> 
> """
> 
> or something like this. It would be nicer if the argument s has a better name,
> I couldn't come up with one though.
> 
> > +    in_macro = match(r'\s*#define', line)
> 
> I think we can move this line to just before the check of single line
> statements (line 1350).

Hopefully all the comments are addressed, but I'd appreciate if you have a
final look.

Regarding in macro: I thought it might be a useful thing for the rest of the
code, but yes, I agree that at least for now we'd better move it closer to the
use.

Thanks a lot for review, Shinichiro!

[David Levin]

(In reply to comment #4)
> (In reply to comment #2) 
> > 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?

Personally when I do a change of this sort, 
1. I run it against a lot of WebKit code (WebCore and JavaScriptCore). 
2. If there are warnings, I'll randomly check around 5 of them to ensure that they are real warnings.
3. If any warnings are false positives, then the checker should be fixed and this process repeated (go to step 1).
It may seem like a bit of overhead but it is worse to accidently flag correct code and then have to deal with people's frustration with that.

[Shinichiro Hamaji]

Comment on attachment 47544 [details]
Next round

Thanks again for your fix! Looks good assuming you'll land this after you check this patch with some code as David suggested (thanks David for your comment, I somehow missed Anton's question). By the way, I think people rarely ignores these rules so I guess checking a lot of source code won't be so painful.

[anton muhin]

Created attachment 47553 [details]
Additionally support lines ending with \---useful for macros

[anton muhin]

(In reply to comment #8)
> (In reply to comment #4)
> > (In reply to comment #2) 
> > > 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?
> 
> Personally when I do a change of this sort, 
> 1. I run it against a lot of WebKit code (WebCore and JavaScriptCore). 
> 2. If there are warnings, I'll randomly check around 5 of them to ensure that
> they are real warnings.
> 3. If any warnings are false positives, then the checker should be fixed and
> this process repeated (go to step 1).
> It may seem like a bit of overhead but it is worse to accidently flag correct
> code and then have to deal with people's frustration with that.

Thanks a lot, David.  So I ran it through all the sources and inspected several warnings, most of them are correct, but I still decided to allow lines terminating with \ as multiline macros are too painful to write.  And \ are kind of legitime C code.

Spotted a nice case of macro to inline function conversion which kept \ at the end :)

[anton muhin]

Created attachment 47555 [details]
Removing traces of incorrect redirects

[Shinichiro Hamaji]

Comment on attachment 47555 [details]
Removing traces of incorrect redirects

> but I still decided to allow lines
> terminating with \ as multiline macros are too painful to write.  And \ are
> kind of legitime C code.
> 
> Spotted a nice case of macro to inline function conversion which kept \ at the
> end :)

Ah, very nice catch. Thanks for doing this check.

[anton muhin]

Comment on attachment 47555 [details]
Removing traces of incorrect redirects

Thanks a lot for review, Shinichiro and David.

[WebKit Commit Bot]

Comment on attachment 47555 [details]
Removing traces of incorrect redirects

Clearing flags on attachment: 47555

Committed r53989: <http://trac.webkit.org/changeset/53989>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.