check-webkit-style -- close_expression function doesn't work correctly.
authorlevin@chromium.org <levin@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 12 Jan 2011 19:41:54 +0000 (19:41 +0000)
committerlevin@chromium.org <levin@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 12 Jan 2011 19:41:54 +0000 (19:41 +0000)
https://bugs.webkit.org/show_bug.cgi?id=52272

Reviewed by Shinichiro Hamaji.

* Scripts/webkitpy/style/checkers/cpp.py:
(Position.__str__): Added a way to convert it to a string which is useful in tests.
(Position.__cmp__): Added a way to compare Position which is useful in tests and
generally useful (for upcoming code changes).
(close_expression): Changed to use Position for input and output.
Also, fixed many bugs such as only working correctly for parenthesis,
not working correctly if given an offset in a line and not finding
the real closing element if there were multiple closing elements in
the same line.
(detect_functions): Adjusted due to the change in arguments for
close_expression.
* Scripts/webkitpy/style/checkers/cpp_unittest.py:
(CppStyleTestBase.assert_positions_equal): Added a way to verify that
two positions are the same.
(CppStyleTest.test_position): Added tests for the __str_ and __cmp__ methods.
(CppStyleTest.test_close_expression): Added tests to catch the issues
that were fixed.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@75628 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Tools/ChangeLog
Tools/Scripts/webkitpy/style/checkers/cpp.py
Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py

index 1fe88ce..34a317f 100644 (file)
@@ -1,3 +1,28 @@
+2011-01-12  David Levin  <levin@chromium.org>
+
+        Reviewed by Shinichiro Hamaji.
+
+        check-webkit-style -- close_expression function doesn't work correctly.
+        https://bugs.webkit.org/show_bug.cgi?id=52272
+
+        * Scripts/webkitpy/style/checkers/cpp.py:
+        (Position.__str__): Added a way to convert it to a string which is useful in tests.
+        (Position.__cmp__): Added a way to compare Position which is useful in tests and
+        generally useful (for upcoming code changes).
+        (close_expression): Changed to use Position for input and output.
+        Also, fixed many bugs such as only working correctly for parenthesis,
+        not working correctly if given an offset in a line and not finding
+        the real closing element if there were multiple closing elements in
+        the same line.
+        (detect_functions): Adjusted due to the change in arguments for
+        close_expression.
+        * Scripts/webkitpy/style/checkers/cpp_unittest.py:
+        (CppStyleTestBase.assert_positions_equal): Added a way to verify that
+        two positions are the same.
+        (CppStyleTest.test_position): Added tests for the __str_ and __cmp__ methods.
+        (CppStyleTest.test_close_expression): Added tests to catch the issues
+        that were fixed.
+
 2011-01-12  Sheriff Bot  <webkit.review.bot@gmail.com>
 
         Unreviewed, rolling out r75576.
index 4ea7d69..bc1fabd 100644 (file)
@@ -344,6 +344,12 @@ class Position(object):
         self.row = row
         self.column = column
 
+    def __str__(self):
+        return '(%s, %s)' % (self.row, self.column)
+
+    def __cmp__(self, other):
+        return self.row.__cmp__(other.row) or self.column.__cmp__(other.column)
+
 
 class Parameter(object):
     """Information about one function parameter."""
@@ -791,49 +797,58 @@ class CleansedLines(object):
         return elided
 
 
-def close_expression(clean_lines, line_number, pos):
+def close_expression(elided, position):
     """If input points to ( or { or [, finds the position that closes it.
 
-    If clean_lines.elided[line_number][pos] points to a '(' or '{' or '[', finds
-    the line_number/pos that correspond to the closing of the expression.
+    If elided[position.row][position.column] points to a '(' or '{' or '[',
+    finds the line_number/pos that correspond to the closing of the expression.
 
-    Args:
-      clean_lines: A CleansedLines instance containing the file.
-      line_number: The number of the line to check.
-      pos: A position on the line.
+     Args:
+       elided: A CleansedLines.elided instance containing the file.
+       position: The position of the opening item.
 
-    Returns:
-      A tuple (line, line_number, pos) pointer *past* the closing brace, or
-      ('', len(clean_lines.elided), -1) if we never find a close.  Note we
-      ignore strings and comments when matching; and the line we return is the
-      'cleansed' line at line_number.
+     Returns:
+      The Position *past* the closing brace, or Position(len(elided), -1)
+      if we never find a close. Note we ignore strings and comments when matching.
     """
-
-    line = clean_lines.elided[line_number]
-    start_character = line[pos]
-    if start_character not in '({[':
-        return (line, clean_lines.num_lines(), -1)
+    line = elided[position.row]
+    start_character = line[position.column]
     if start_character == '(':
-        end_character = ')'
-    if start_character == '[':
-        end_character = ']'
-    if start_character == '{':
-        end_character = '}'
-
-    num_open = line.count(start_character) - line.count(end_character)
-    while num_open > 0:
+        enclosing_character_regex = r'[\(\)]'
+    elif start_character == '[':
+        enclosing_character_regex = r'[\[\]]'
+    elif start_character == '{':
+        enclosing_character_regex = r'[\{\}]'
+    else:
+        return Position(len(elided), -1)
+
+    current_column = position.column + 1
+    line_number = position.row
+    net_open = 1
+    for line in elided[position.row:]:
+        line = line[current_column:]
+
+        # Search the current line for opening and closing characters.
+        while True:
+            next_enclosing_character = search(enclosing_character_regex, line)
+            # No more on this line.
+            if not next_enclosing_character:
+                break
+            current_column += next_enclosing_character.end(0)
+            line = line[next_enclosing_character.end(0):]
+            if next_enclosing_character.group(0) == start_character:
+                net_open += 1
+            else:
+                net_open -= 1
+                if not net_open:
+                    return Position(line_number, current_column)
+
+        # Proceed to the next line.
         line_number += 1
-        if line_number >= clean_lines.num_lines():
-            return ('', len(clean_lines.elided), -1)
-        line = clean_lines.elided[line_number]
-        num_open += line.count(start_character) - line.count(end_character)
-    # OK, now find the end_character that actually got us back to even
-    endpos = len(line)
-    while num_open >= 0:
-        endpos = line.rfind(')', 0, endpos)
-        num_open -= 1                 # chopped off another )
-    return (line, line_number, endpos + 1)
+        current_column = 0
 
+    # The given item was not closed.
+    return Position(len(elided), -1)
 
 def check_for_copyright(lines, error):
     """Logs an error if no Copyright message appears at the top of the file."""
@@ -1429,18 +1444,17 @@ def detect_functions(clean_lines, line_number, function_state, error):
                 function += '()'
 
             parameter_start_position = Position(line_number, match_function.end(1))
-            close_result = close_expression(clean_lines, line_number, parameter_start_position.column)
-            if close_result[1] == len(clean_lines.elided):
+            parameter_end_position = close_expression(clean_lines.elided, parameter_start_position)
+            if parameter_end_position.row == len(clean_lines.elided):
                 # No end was found.
                 return
-            parameter_end_position = Position(close_result[1], close_result[2])
 
             is_declaration = bool(search(r'^[^{]*;', start_line))
             if is_declaration:
                 ending_line_number = start_line_number
             else:
                 open_brace_index = start_line.find('{')
-                ending_line_number = close_expression(clean_lines, start_line_number, open_brace_index)[1]
+                ending_line_number = close_expression(clean_lines.elided, Position(start_line_number, open_brace_index)).row
             function_state.begin(function, start_line_number, ending_line_number, is_declaration,
                                  parameter_start_position, parameter_end_position, clean_lines)
             return
index d39d2ba..614445f 100644 (file)
@@ -342,6 +342,14 @@ class CppStyleTestBase(unittest.TestCase):
                 'Blank line at the end of a code block.  Is this needed?'
                 '  [whitespace/blank_line] [3]'))
 
+    def assert_positions_equal(self, position, tuple_position):
+        """Checks if the two positions are equal.
+
+        position: a cpp_style.Position object.
+        tuple_position: a tuple (row, column) to compare against."""
+        self.assertEquals(position, cpp_style.Position(tuple_position[0], tuple_position[1]),
+                          'position %s, tuple_position %s' % (position, tuple_position))
+
 
 class FunctionDetectionTest(CppStyleTestBase):
     def perform_function_detection(self, lines, function_information):
@@ -523,6 +531,24 @@ class CppStyleTest(CppStyleTestBase):
         cpp_style.remove_multi_line_comments_from_range(lines, 1, 4)
         self.assertEquals(['a', '// dummy', '// dummy', '// dummy', 'b'], lines)
 
+    def test_position(self):
+        position = cpp_style.Position(3, 4)
+        self.assert_positions_equal(position, (3, 4))
+        self.assertEquals(position.row, 3)
+        self.assertTrue(position > cpp_style.Position(position.row - 1, position.column + 1))
+        self.assertTrue(position > cpp_style.Position(position.row, position.column - 1))
+        self.assertTrue(position < cpp_style.Position(position.row, position.column + 1))
+        self.assertTrue(position < cpp_style.Position(position.row + 1, position.column - 1))
+        self.assertEquals(position.__str__(), '(3, 4)')
+
+    def test_close_expression(self):
+        self.assertEquals(cpp_style.Position(1, -1), cpp_style.close_expression([')('], cpp_style.Position(0, 1)))
+        self.assertEquals(cpp_style.Position(1, -1), cpp_style.close_expression([') ()'], cpp_style.Position(0, 1)))
+        self.assertEquals(cpp_style.Position(0, 4), cpp_style.close_expression([')[)]'], cpp_style.Position(0, 1)))
+        self.assertEquals(cpp_style.Position(0, 5), cpp_style.close_expression(['}{}{}'], cpp_style.Position(0, 3)))
+        self.assertEquals(cpp_style.Position(1, 1), cpp_style.close_expression(['}{}{', '}'], cpp_style.Position(0, 3)))
+        self.assertEquals(cpp_style.Position(2, -1), cpp_style.close_expression(['][][', ' '], cpp_style.Position(0, 3)))
+
     def test_spaces_at_end_of_line(self):
         self.assert_lint(
             '// Hello there ',