Changeset 53989 in webkit

Timestamp:

Jan 28, 2010 12:29:05 AM (14 years ago)

Author:

eric@webkit.org

Message:

2010-01-28 Anton Muhin <​antonm@chromium.org>

Reviewed by Shinichiro Hamaji.

Improve treatment of conditions and rest of the line for if, else, switch and alikes
​https://bugs.webkit.org/show_bug.cgi?id=34173

• Scripts/webkitpy/style/cpp_style.py:
• Scripts/webkitpy/style/cpp_style_unittest.py:

Location:

trunk/WebKitTools

Files:

• ChangeLog (modified) (1 diff)
• Scripts/webkitpy/style/processors/cpp.py (modified) (2 diffs)
• Scripts/webkitpy/style/processors/cpp_unittest.py (modified) (4 diffs)

trunk/WebKitTools/ChangeLog

@@ +1 @@
+2010-01-28  Anton Muhin  <antonm@chromium.org>
+
+        Reviewed by Shinichiro Hamaji.
+
+        Improve treatment of conditions and rest of the line for if, else, switch and alikes
+        https://bugs.webkit.org/show_bug.cgi?id=34173
+
+        * Scripts/webkitpy/style/cpp_style.py:
+        * Scripts/webkitpy/style/cpp_style_unittest.py:
+
 2010-01-28  Joe Mason  <jmason@rim.com>
 

trunk/WebKitTools/Scripts/webkitpy/style/processors/cpp.py

@@ -152 +152 @@
     return _regexp_compile_cache[pattern].subn(replacement, s)
 
+
+def up_to_unmatched_closing_paren(s):
+    """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 (prefix before first unmatched ')',
+      reminder of s after first unmatched ')'), e.g.,
+      up_to_unmatched_closing_paren("a == (b + c)) { ")
+      returns "a == (b + c)", " {".
+      Returns None, None if there is no unmatched ')'
+
+    """
+    i = 1
+    for pos, c in enumerate(s):
+      if c == '(':
+        i += 1
+      elif c == ')':
+        i -= 1
+        if i == 0:
+          return s[:pos], s[pos + 1:]
+    return None, None
 
 class _IncludeState(dict):
@@ -1310 +1335 @@
     # We don't want: "if ( foo)" or "if ( foo   )".
     # Exception: "for ( ; foo; bar)" and "for (foo; bar; )" are allowed.
-    matched = search(r'\b(if|for|foreach|while|switch)\s*\(([ ]*)(.).*[^ ]+([ ]*)\)\s*{\s*$',
-                     line)
+    matched = search(r'\b(?P<statement>if|for|foreach|while|switch)\s*\((?P<reminder>.*)$', line)
     if matched:
-        if len(matched.group(2)) != len(matched.group(4)):
-            if not (matched.group(3) == ';'
-                    and len(matched.group(2)) == 1 + len(matched.group(4))
-                    or not matched.group(2) and search(r'\bfor\s*\(.*; \)', line)):
-                error(line_number, 'whitespace/parens', 5,
-                      'Mismatching spaces inside () in %s' % matched.group(1))
-        if not len(matched.group(2)) in [0, 1]:
-            error(line_number, 'whitespace/parens', 5,
-                  'Should have zero or one spaces inside ( and ) in %s' %
-                  matched.group(1))
+        statement = matched.group('statement')
+        condition, rest = up_to_unmatched_closing_paren(matched.group('reminder'))
+        if condition is not None:
+            condition_match = search(r'(?P<leading>[ ]*)(?P<separator>.).*[^ ]+(?P<trailing>[ ]*)', condition)
+            if condition_match:
+                n_leading = len(condition_match.group('leading'))
+                n_trailing = len(condition_match.group('trailing'))
+                if n_leading != n_trailing:
+                    for_exception = statement == 'for' and (
+                        (condition.startswith(' ;') and n_trailing == 0) or
+                        (condition.endswith('; ')   and n_leading == 0))
+                    if not for_exception:
+                        error(line_number, 'whitespace/parens', 5,
+                              'Mismatching spaces inside () in %s' % statement)
+                if n_leading > 1:
+                    error(line_number, 'whitespace/parens', 5,
+                          'Should have zero or one spaces inside ( and ) in %s' %
+                          statement)
+
+            # Do not check for more than one command in macros
+            in_macro = match(r'\s*#define', line)
+            if not in_macro and not match(r'((\s*{\s*}?)|(\s*;?))\s*\\?$', rest):
+                error(line_number, 'whitespace/parens', 4,
+                      'More than one command on the same line in %s' % statement)
 
     # You should always have a space after a comma (either as fn arg or operator)

trunk/WebKitTools/Scripts/webkitpy/style/processors/cpp_unittest.py

@@ -1168 +1168 @@
         self.assert_lint('for (foo; ba; bar ) {', 'Mismatching spaces inside () in for'
                          '  [whitespace/parens] [5]')
+        self.assert_lint('for ((foo); (ba); (bar) ) {', 'Mismatching spaces inside () in for'
+                         '  [whitespace/parens] [5]')
         self.assert_lint('for (; foo; bar) {', '')
+        self.assert_lint('for (; (foo); (bar)) {', '')
         self.assert_lint('for ( ; foo; bar) {', '')
+        self.assert_lint('for ( ; (foo); (bar)) {', '')
         self.assert_lint('for ( ; foo; bar ) {', '')
+        self.assert_lint('for ( ; (foo); (bar) ) {', '')
         self.assert_lint('for (foo; bar; ) {', '')
+        self.assert_lint('for ((foo); (bar); ) {', '')
         self.assert_lint('foreach (foo, foos ) {', 'Mismatching spaces inside () in foreach'
                          '  [whitespace/parens] [5]')
@@ -2999 +3005 @@
             '')
         self.assert_multi_line_lint(
+            '    if (condition) \\\n'
+            '        doIt();\n',
+            '')
+        self.assert_multi_line_lint(
             '    x++; y++;',
             'More than one command on the same line  [whitespace/newline] [4]')
-        # FIXME: Make this fail.
-        # self.assert_multi_line_lint(
-        #     '    if (condition) doIt();\n',
-        #     '')
+        self.assert_multi_line_lint(
+            '    if (condition) doIt();\n',
+            'More than one command on the same line in if  [whitespace/parens] [4]')
 
         # 2. An else statement should go on the same line as a preceding
@@ -3035 +3044 @@
             '#define TEST_ASSERT(expression) do { if (!(expression)) { TestsController::shared().testFailed(__FILE__, __LINE__, #expression); return; } } while (0)\n',
             '')
+        self.assert_multi_line_lint(
+            '#define TEST_ASSERT(expression) do { if ( !(expression)) { TestsController::shared().testFailed(__FILE__, __LINE__, #expression); return; } } while (0)\n',
+            'Mismatching spaces inside () in if  [whitespace/parens] [5]')
+        # FIXME: currently we only check first conditional, so we cannot detect errors in next ones.
+        # self.assert_multi_line_lint(
+        #     '#define TEST_ASSERT(expression) do { if (!(expression)) { TestsController::shared().testFailed(__FILE__, __LINE__, #expression); return; } } while (0 )\n',
+        #     'Mismatching spaces inside () in if  [whitespace/parens] [5]')
         self.assert_multi_line_lint(
             'if (condition) {\n'
@@ -3048 +3064 @@
             'if (condition) doSomething(); else doSomethingElse();\n',
             ['More than one command on the same line  [whitespace/newline] [4]',
-             'Else clause should never be on same line as else (use 2 lines)  [whitespace/newline] [4]'])
-        # FIXME: Make this fail.
-        # self.assert_multi_line_lint(
-        #     'if (condition) doSomething(); else {\n'
-        #     '    doSomethingElse();\n'
-        #     '}\n',
-        #     '')
+             'Else clause should never be on same line as else (use 2 lines)  [whitespace/newline] [4]',
+             'More than one command on the same line in if  [whitespace/parens] [4]'])
+        self.assert_multi_line_lint(
+            'if (condition) doSomething(); else {\n'
+            '    doSomethingElse();\n'
+            '}\n',
+            ['More than one command on the same line in if  [whitespace/parens] [4]',
+             'One line control clauses should not use braces.  [whitespace/braces] [4]'])
 
         # 3. An else if statement should be written as an if statement