Bug 52272 – check-webkit-style -- close_expression function doesn't work correctly.

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

[David Levin]

See summary.

[David Levin]

Created attachment 78644 [details]
Patch

[David Levin]

Comment on attachment 78644 [details]
Patch

At least one person said this was hard to understand so I'll try to make the code easier to read.

[Shinichiro Hamaji]

Comment on attachment 78644 [details]
Patch

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

No, I was so sick and my brain stopped working when I told you that I couldn't understand it. Now I feel better and I can understand this patch :) I'm sure this code is good enough to land this as is. I put some nitpicks.

> Tools/Scripts/webkitpy/style/checkers/cpp.py:817
>          end_character = ')'

How about creating the regexp string here?

if start_character == '(':
  enclosing_characeter_regexp = r'[\{\}]'
elif
  ...

> Tools/Scripts/webkitpy/style/checkers/cpp.py:829
> +    while True:

I think you used while because we start in the middle of the first line, but I'd mildly prefer for-loop:

current_column = position.column + 1
net_open = 1
for line_number in xrange(position.row, len(elided):
  line = elided[line_number][current_column:]
  while True:
     ...
  current_column = 0
return Position(len(elided), -1)

because in this way we can clearly see the iterator of the outer loop.

[David Levin]

Committed as http://trac.webkit.org/changeset/75628

[David Levin]

(In reply to comment #3)
> (From update of attachment 78644 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78644&action=review
> 
> No, I was so sick and my brain stopped working when I told you that I couldn't understand it. Now I feel better and I can understand this patch :) I'm sure this code is good enough to land this as is. I put some nitpicks.
> 
> > Tools/Scripts/webkitpy/style/checkers/cpp.py:817
> >          end_character = ')'
> 
> How about creating the regexp string here?
> 
> if start_character == '(':
>   enclosing_characeter_regexp = r'[\{\}]'
> elif
>   ...
> 
Done.

> > Tools/Scripts/webkitpy/style/checkers/cpp.py:829
> > +    while True:
> 
> I think you used while because we start in the middle of the first line, but I'd mildly prefer for-loop:
> 
> current_column = position.column + 1
> net_open = 1
> for line_number in xrange(position.row, len(elided):
>   line = elided[line_number][current_column:]
>   while True:
>      ...
>   current_column = 0
> return Position(len(elided), -1)
> 
> because in this way we can clearly see the iterator of the outer loop.

I really like this. I did it (in a slightly modified way but basically the same idea).