[webkit-reviews] review granted: [Bug 51451] check-webkit-style should be able to parse function declaration parameters. : [Attachment 77394] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted David Levin <levin at chromium.org>'s
request for review:
Bug 51451: check-webkit-style should be able to parse function declaration
parameters.
https://bugs.webkit.org/show_bug.cgi?id=51451

Attachment 77394: Patch
https://bugs.webkit.org/attachment.cgi?id=77394&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.