Bug 51451 – check-webkit-style should be able to parse function declaration parameters.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Levin]

See summary.

[David Levin]

Created attachment 77192 [details]
Patch

[David Levin]

Comment on attachment 77192 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77192&action=review

> Tools/Scripts/webkitpy/style/checkers/cpp.py:371
> +        # to ensure only the parameters remain without the surrounding paranthesis.

typo: paranthesis

> Tools/Scripts/webkitpy/style/checkers/cpp.py:1322
> +

extra blank line

[David Levin]

Comment on attachment 77192 [details]
Patch

Making some improvements.

[David Levin]

Created attachment 77282 [details]
Patch

[David Levin]

Created attachment 77283 [details]
Patch

[WebKit Review Bot]

Attachment 77282 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/style/checkers/cpp.py', u'Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py']" exit_code: 1
Tools/Scripts/webkitpy/style/checkers/cpp.py:391:  whitespace before ')'  [pep8/E202] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

Created attachment 77308 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 77308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77308&action=review

Should we have a separate helper class for this? Seems like a lot of logic.  Or at least we could break thsi huge new function down into smaller parts.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:352
> +    def get_parameter_list(self):

Do we really use get in names?

This function is also huge :((

[David Levin]

Comment on attachment 77308 [details]
Patch

Need to address comments.

[David Levin]

Created attachment 77391 [details]
Patch

[WebKit Review Bot]

Attachment 77391 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/style/checkers/cpp.py', u'Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py']" exit_code: 1
Tools/Scripts/webkitpy/style/checkers/cpp.py:310:  expected 2 blank lines, found 1  [pep8/E302] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Levin]

Created attachment 77394 [details]
Patch

[David Levin]

(In reply to comment #11)
> Attachment 77391 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy/style/checkers/cpp.py', u'Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py']" exit_code: 1
> Tools/Scripts/webkitpy/style/checkers/cpp.py:310:  expected 2 blank lines, found 1  [pep8/E302] [5]
> Total errors found: 1 in 3 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

I should change the webkit-patch upload to warn you if you have local changes that aren't in the change or something like that :( (The style step warned me. I fixed it and then uploaded without committing first.)

[David Levin]

(In reply to comment #8)
> (From update of attachment 77308 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77308&action=review
> 
> Should we have a separate helper class for this? Seems like a lot of logic.  Or at least we could break thsi huge new function down into smaller parts.
> 
> > Tools/Scripts/webkitpy/style/checkers/cpp.py:352
> > +    def get_parameter_list(self):
> 
> Do we really use get in names?

Oops. fixed.

> This function is also huge :((

If you think things are still too huge or not understand, let me know and I'll work to make it better.

At least, it looks a lot better than before to me.

[Eric Seidel (no email)]

Comment on attachment 77394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77394&action=review

I don't fully understand this, but I'm also willing to rs=me.  This is definitely a very cool addition to our style-checker.

You might consider making it a generator instead.  It would simplify the code ever-so-slightly due to not needing current or current_parameter.

I guess the main part I don't undestand is PameterIterator.__init__

Tentative r+, but I'd also happily go another round if you have it in you. :)

> Tools/Scripts/webkitpy/style/checkers/cpp.py:322
> +    def __init__(self, clean_lines, parameters_start, parameters_end):

I assume start/end are offsets in a string?

> Tools/Scripts/webkitpy/style/checkers/cpp.py:327
> +        # Form a single string with all parameters. Trimming the first and last lines
> +        # to ensure only the parameters remain without the surrounding parenthesis.
> +        parameters_lines = clean_lines.elided[self._starting_line:parameters_end[0] + 1]
> +        parameters_lines[-1] = parameters_lines[-1][:parameters_end[1] - 1]

I'm confused as to what this does? parameters_lines[-1] = parameters_lines[-1]?

> Tools/Scripts/webkitpy/style/checkers/cpp.py:329
> +        # Form a single string with all parameters. Trimming the first and last lines
> +        # to ensure only the parameters remain without the surrounding parenthesis.
> +        parameters_lines = clean_lines.elided[self._starting_line:parameters_end[0] + 1]
> +        parameters_lines[-1] = parameters_lines[-1][:parameters_end[1] - 1]
>  
>  
> +        parameters_lines[0] = parameters_lines[0][parameters_start[1] + 1:]
> +        self._all_parameters = ' '.join(parameters_lines)

This could be a separate function and unit tested.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:402
> +        self._skeleton_parameters = skeleton_parameters

Cleaner woudl be to just return the skeleton parameters here and initialize the self. in teh caller.

Then this is also much easier to unit test.  Would be nice to see some unit tests for this function. :)

> Tools/Scripts/webkitpy/style/checkers/cpp.py:449
> +            parameter_parser = ParameterIterator(self._clean_lines, self.parameter_start, self.parameter_end)
> +            while parameter_parser.next():

The python way would be a generator instead of an explicit current function. :)
http://docs.python.org/tutorial/classes.html#generators

But this is also fine as-is. :)

Then you would just do parameter_list(self._clean_lines, ...) and that would be it.  You could wrap it in tuple() if you wanted to convert it all at once.

[David Levin]

Comment on attachment 77394 [details]
Patch

I'll address feedback and do another round.

[Shinichiro Hamaji]

Comment on attachment 77394 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77394&action=review

A few more comments in addition to Eric's.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:313
> +    def __init__(self, parameter, type_name_index, line_number):

Is the type_name_index good name? I guess parameter_name_index would be better.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:316
> +        self.name = sub(r'=.*', '', parameter[type_name_index:]).strip()

Do we need this line? It looks like line 386 is doing this job already.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:399
> +        if skeleton_parameters.strip():

I couldn't understand the intention of this if-clause at first. We might want a comment for this.

[David Levin]

Created attachment 77451 [details]
Patch

[David Levin]

Created attachment 77456 [details]
Patch

[David Levin]

Comment on attachment 77456 [details]
Patch

I believe that all feedback has been addressed.

[David Levin]

(In reply to comment #17)
> (From update of attachment 77394 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77394&action=review

> > Tools/Scripts/webkitpy/style/checkers/cpp.py:316
> > +        self.name = sub(r'=.*', '', parameter[type_name_index:]).strip()
> 
> Do we need this line? It looks like line 386 is doing this job already.

True. Line 386 did the same thing but that was for the purpose of creating the skeleton parameters. This is operating on the original parameters. At this point, the code operates on the original parameters in order to extra the names without the modifications done during the process of creating skeleton parameters. (Of course this part of the change was still useful.)

[Eric Seidel (no email)]

Comment on attachment 77456 [details]
Patch

OK.

[WebKit Commit Bot]

Comment on attachment 77456 [details]
Patch

Clearing flags on attachment: 77456

Committed r74670: <http://trac.webkit.org/changeset/74670>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74670 might have broken GTK Linux 32-bit Release
The following tests are not passing:
media/controls-strict.html