2011-06-22 David Levin <levin@chromium.org>
authorlevin@chromium.org <levin@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 23 Jun 2011 01:05:17 +0000 (01:05 +0000)
committerlevin@chromium.org <levin@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 23 Jun 2011 01:05:17 +0000 (01:05 +0000)
        Reviewed by Adam Barth.

        check-webkit-style should detect returning (Own|Ref)Ptr instead of the Pass*Ptr version.
        https://bugs.webkit.org/show_bug.cgi?id=63204

        * Scripts/webkitpy/style/checkers/cpp.py: Added a check for the return value and combined
          with similar code for the parameter checking.
        * Scripts/webkitpy/style/checkers/cpp_unittest.py: Removed pass_ptr checks from
          those done for single lines since they don't make sense in that case (variable decls look like function decls).
          Removed some redundant comments (one of which was slightly wrong).
          Added checks for the new functionality and minor other test changes.

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

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

index d1bfb38..0e3fec1 100644 (file)
@@ -1,3 +1,17 @@
+2011-06-22  David Levin  <levin@chromium.org>
+
+        Reviewed by Adam Barth.
+
+        check-webkit-style should detect returning (Own|Ref)Ptr instead of the Pass*Ptr version.
+        https://bugs.webkit.org/show_bug.cgi?id=63204
+
+        * Scripts/webkitpy/style/checkers/cpp.py: Added a check for the return value and combined
+          with similar code for the parameter checking.
+        * Scripts/webkitpy/style/checkers/cpp_unittest.py: Removed pass_ptr checks from
+          those done for single lines since they don't make sense in that case (variable decls look like function decls).
+          Removed some redundant comments (one of which was slightly wrong).
+          Added checks for the new functionality and minor other test changes.
+
 2011-06-22  Nate Chapin  <japhet@chromium.org>
 
         Reviewed by Adam Barth.
 2011-06-22  Nate Chapin  <japhet@chromium.org>
 
         Reviewed by Adam Barth.
index 238f70d..0946327 100644 (file)
@@ -1566,6 +1566,24 @@ def _check_parameter_name_against_text(parameter, text, error):
     return True
 
 
     return True
 
 
+def check_function_definition_and_pass_ptr(type_text, row, location_description, error):
+    """Check that function definitions for use Pass*Ptr instead of *Ptr.
+
+    Args:
+       type_text: A string containing the type. (For return values, it may contain more than the type.)
+       row: The row number of the type.
+       location_description: Used to indicate where the type is. This is either 'parameter' or 'return'.
+       error: The function to call with any errors found.
+    """
+    match_ref_or_own_ptr = '(?=\W|^)(Ref|Own)Ptr(?=\W)'
+    bad_type_usage = search(match_ref_or_own_ptr, type_text)
+    if not bad_type_usage:
+        return
+    type_name = bad_type_usage.group(0)
+    error(row, 'readability/pass_ptr', 5,
+          'The %s type should use Pass%s instead of %s.' % (location_description, type_name, type_name))
+
+
 def check_function_definition(filename, file_extension, clean_lines, line_number, function_state, error):
     """Check that function definitions for style issues.
 
 def check_function_definition(filename, file_extension, clean_lines, line_number, function_state, error):
     """Check that function definitions for style issues.
 
@@ -1597,13 +1615,11 @@ def check_function_definition(filename, file_extension, clean_lines, line_number
             error(function_state.function_name_start_position.row, 'readability/webkit_api', 5,
                   'WEBKIT_API should not be used with a pure virtual function.')
 
             error(function_state.function_name_start_position.row, 'readability/webkit_api', 5,
                   'WEBKIT_API should not be used with a pure virtual function.')
 
+    check_function_definition_and_pass_ptr(modifiers_and_return_type, function_state.function_name_start_position.row, 'return', error)
+
     parameter_list = function_state.parameter_list()
     for parameter in parameter_list:
     parameter_list = function_state.parameter_list()
     for parameter in parameter_list:
-        bad_parameter_type = search('(?=\W|^)(Ref|Own)Ptr(?=\W)', parameter.type)
-        if bad_parameter_type:
-            type_name = bad_parameter_type.group(0)
-            error(parameter.row, 'readability/pass_ptr', 5,
-                  'The parameter type should use Pass%s instead of %s.' % (type_name, type_name))
+        check_function_definition_and_pass_ptr(parameter.type, parameter.row, 'parameter', error)
 
         # Do checks specific to function declarations and parameter names.
         if not function_state.is_declaration or not parameter.name:
 
         # Do checks specific to function declarations and parameter names.
         if not function_state.is_declaration or not parameter.name:
index ee2f5ea..800e2e3 100644 (file)
@@ -259,6 +259,7 @@ class CppStyleTestBase(unittest.TestCase):
                              '-legal/copyright',
                              '-readability/fn_size',
                              '-readability/parameter_name',
                              '-legal/copyright',
                              '-readability/fn_size',
                              '-readability/parameter_name',
+                             '-readability/pass_ptr',
                              '-whitespace/ending_newline')
         return self.perform_lint(code, filename, basic_error_rules)
 
                              '-whitespace/ending_newline')
         return self.perform_lint(code, filename, basic_error_rules)
 
@@ -3205,7 +3206,6 @@ class PassPtrTest(CppStyleTestBase):
                           self.perform_pass_ptr_check(code))
 
     def test_pass_ref_ptr_in_function(self):
                           self.perform_pass_ptr_check(code))
 
     def test_pass_ref_ptr_in_function(self):
-        # Local variables should never be PassRefPtr.
         self.assert_pass_ptr_check(
             'int myFunction()\n'
             '{\n'
         self.assert_pass_ptr_check(
             'int myFunction()\n'
             '{\n'
@@ -3215,7 +3215,6 @@ class PassPtrTest(CppStyleTestBase):
             'http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]')
 
     def test_pass_own_ptr_in_function(self):
             'http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]')
 
     def test_pass_own_ptr_in_function(self):
-        # Local variables should never be PassRefPtr.
         self.assert_pass_ptr_check(
             'int myFunction()\n'
             '{\n'
         self.assert_pass_ptr_check(
             'int myFunction()\n'
             '{\n'
@@ -3225,7 +3224,6 @@ class PassPtrTest(CppStyleTestBase):
             'http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]')
 
     def test_pass_other_type_ptr_in_function(self):
             'http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]')
 
     def test_pass_other_type_ptr_in_function(self):
-        # Local variables should never be PassRefPtr.
         self.assert_pass_ptr_check(
             'int myFunction()\n'
             '{\n'
         self.assert_pass_ptr_check(
             'int myFunction()\n'
             '{\n'
@@ -3249,15 +3247,26 @@ class PassPtrTest(CppStyleTestBase):
         self.assert_pass_ptr_check(
             'PassRefPtr<Type1> myFunction();\n',
             '')
         self.assert_pass_ptr_check(
             'PassRefPtr<Type1> myFunction();\n',
             '')
+        self.assert_pass_ptr_check(
+            'OwnRefPtr<Type1> myFunction();\n',
+            '')
+        self.assert_pass_ptr_check(
+            'RefPtr<Type1> myFunction(int)\n'
+            '{\n'
+            '}',
+            'The return type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]')
+        self.assert_pass_ptr_check(
+            'OwnPtr<Type1> myFunction(int)\n'
+            '{\n'
+            '}',
+            'The return type should use PassOwnPtr instead of OwnPtr.  [readability/pass_ptr] [5]')
 
 
-    def test_pass_ref_ptr_parameter_value(self):
+    def test_ref_ptr_parameter_value(self):
         self.assert_pass_ptr_check(
             'int myFunction(PassRefPtr<Type1>)\n'
             '{\n'
             '}',
             '')
         self.assert_pass_ptr_check(
             'int myFunction(PassRefPtr<Type1>)\n'
             '{\n'
             '}',
             '')
-
-    def test_ref_ptr_parameter_value(self):
         self.assert_pass_ptr_check(
             'int myFunction(RefPtr<Type1>)\n'
             '{\n'
         self.assert_pass_ptr_check(
             'int myFunction(RefPtr<Type1>)\n'
             '{\n'
@@ -3266,6 +3275,11 @@ class PassPtrTest(CppStyleTestBase):
 
     def test_own_ptr_parameter_value(self):
         self.assert_pass_ptr_check(
 
     def test_own_ptr_parameter_value(self):
         self.assert_pass_ptr_check(
+            'int myFunction(PassOwnPtr<Type1>)\n'
+            '{\n'
+            '}',
+            '')
+        self.assert_pass_ptr_check(
             'int myFunction(OwnPtr<Type1>)\n'
             '{\n'
             '}',
             'int myFunction(OwnPtr<Type1>)\n'
             '{\n'
             '}',