Changeset 200859 in webkit

Timestamp:

May 13, 2016, 9:16:51 AM (9 years ago)

Author:

beidson@apple.com

Message:

Protector Ref/RefPtrs should have a specified naming style.
​https://bugs.webkit.org/show_bug.cgi?id=157591

Reviewed by Darin Adler.

Tools:

• Scripts/webkitpy/style/checkers/cpp.py:

(check_identifier_name_in_declaration):
(CppChecker):

• Scripts/webkitpy/style/checkers/cpp_unittest.py:

(WebKitStyleTest.test_names):

Websites/webkit.org:

• code-style.md:

Location:

trunk

Files:

• Tools/ChangeLog (modified) (1 diff)
• Tools/Scripts/webkitpy/style/checkers/cpp.py (modified) (2 diffs)
• Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py (modified) (1 diff)
• Websites/webkit.org/ChangeLog (modified) (1 diff)
• Websites/webkit.org/code-style.md (modified) (1 diff)

trunk/Tools/ChangeLog

@@ +1 @@
+2016-05-13  Brady Eidson  <beidson@apple.com>
+
+        Protector Ref/RefPtrs should have a specified naming style.
+        https://bugs.webkit.org/show_bug.cgi?id=157591
+
+        Reviewed by Darin Adler.
+
+        * Scripts/webkitpy/style/checkers/cpp.py:
+        (check_identifier_name_in_declaration):
+        (CppChecker):
+        * Scripts/webkitpy/style/checkers/cpp_unittest.py:
+        (WebKitStyleTest.test_names):
+
 2016-05-12  Csaba Osztrogonác  <ossy@webkit.org>
 

trunk/Tools/Scripts/webkitpy/style/checkers/cpp.py

@@ -3292 +3292 @@
         return
 
+    # Make sure Ref/RefPtrs used as protectors are named correctly, and do this before we start stripping things off the input.
+    ref_regexp = r'^\s*Ref(Ptr)?<([\w_]|::)+> (?P<protector_name>[\w_]+)\((\*|&)*(m_)?(?P<protected_name>[\w_]+)\);'
+    ref_check = match(ref_regexp, line)
+    if ref_check:
+        protector_name = ref_check.group('protector_name')
+        protected_name = ref_check.group('protected_name')
+        cap_protected_name = protected_name[0].upper() + protected_name[1:]
+        expected_protector_name = 'protected' + cap_protected_name
+        if protected_name == 'this' and protector_name != 'protectedThis':
+            error(line_number, 'readability/naming/protected', 4, "\'" + protector_name + "\' is incorrectly named. It should be named \'protectedThis\'.")
+        elif protector_name == expected_protector_name or protector_name == 'protector':
+            return
+        else:
+            error(line_number, 'readability/naming/protected', 4, "\'" + protector_name + "\' is incorrectly named. It should be named \'protector\' or \'" + expected_protector_name + "\'.")
+
     # Basically, a declaration is a type name followed by whitespaces
     # followed by an identifier. The type name can be complicated
@@ -3847 +3862 @@
         'readability/parameter_name',
         'readability/naming',
+        'readability/naming/protected',
         'readability/naming/underscores',
         'readability/null',

trunk/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py

@@ -5180 +5180 @@
                          '_fillRule' + name_underscore_error_message)
 
+        # Valid protector RefPtr/Ref variable names.
+        self.assert_lint('RefPtr<Node> protectedThis(this);', '')
+        self.assert_lint('Ref<Node> protectedThis(*this);', '')
+        self.assert_lint('RefPtr<Node> protectedNode(node);', '')
+        self.assert_lint('RefPtr<Node> protectedNode(&node);', '')
+        self.assert_lint('RefPtr<Node> protector(node);', '')
+        self.assert_lint('RefPtr<Node> protector(&node);', '')
+        self.assert_lint('Ref<Node> protectedNode(node);', '')
+        self.assert_lint('Ref<Node> protectedNode(*node);', '')
+        self.assert_lint('Ref<Node> protector(node);', '')
+        self.assert_lint('Ref<Node> protector(*node);', '')
+        self.assert_lint('RefPtr<Node> protectedOtherNode(otherNode);', '')
+        self.assert_lint('RefPtr<Node> protectedOtherNode(&otherNode);', '')
+        self.assert_lint('Ref<Node> protectedOtherNode(otherNode);', '')
+        self.assert_lint('Ref<Node> protectedOtherNode(*otherNode);', '')
+        self.assert_lint('RefPtr<Widget> protectedWidget(m_widget);', '')
+        self.assert_lint('RefPtr<Widget> protectedWidget(&m_widget);', '')
+        self.assert_lint('Ref<Widget> protectedWidget(m_widget);', '')
+        self.assert_lint('Ref<Widget> protectedWidget(*m_widget);', '')
+        self.assert_lint('RefPtr<Widget> protector(m_widget);', '')
+        self.assert_lint('RefPtr<Widget> protector(&m_widget);', '')
+        self.assert_lint('Ref<Widget> protector(m_widget);', '')
+        self.assert_lint('Ref<Widget> protector(*m_widget);', '')
+        self.assert_lint('RefPtr<SomeNamespace::Node> protectedThis(this);', '')
+        self.assert_lint('RefPtr<SomeClass::InternalClass::Node> protectedThis(this);', '')
+
+        # Invalid protector RefPtr/Ref variable names.
+        self.assert_lint('RefPtr<Node> protector(this);', "'protector' is incorrectly named. It should be named 'protectedThis'.  [readability/naming/protected] [4]")
+        self.assert_lint('Ref<Node> protector(*this);', "'protector' is incorrectly named. It should be named 'protectedThis'.  [readability/naming/protected] [4]")
+        self.assert_lint('RefPtr<Node> self(this);', "'self' is incorrectly named. It should be named 'protectedThis'.  [readability/naming/protected] [4]")
+        self.assert_lint('Ref<Node> self(*this);', "'self' is incorrectly named. It should be named 'protectedThis'.  [readability/naming/protected] [4]")
+        self.assert_lint('RefPtr<Node> protectedThis(node);', "'protectedThis' is incorrectly named. It should be named 'protector' or 'protectedNode'.  [readability/naming/protected] [4]")
+        self.assert_lint('RefPtr<Node> protectedThis(&node);', "'protectedThis' is incorrectly named. It should be named 'protector' or 'protectedNode'.  [readability/naming/protected] [4]")
+        self.assert_lint('Ref<Node> protectedThis(node);', "'protectedThis' is incorrectly named. It should be named 'protector' or 'protectedNode'.  [readability/naming/protected] [4]")
+        self.assert_lint('Ref<Node> protectedThis(*node);', "'protectedThis' is incorrectly named. It should be named 'protector' or 'protectedNode'.  [readability/naming/protected] [4]")
+        self.assert_lint('RefPtr<Node> protectedNode(otherNode);', "'protectedNode' is incorrectly named. It should be named 'protector' or 'protectedOtherNode'.  [readability/naming/protected] [4]")
+        self.assert_lint('RefPtr<Node> protectedNode(&otherNode);', "'protectedNode' is incorrectly named. It should be named 'protector' or 'protectedOtherNode'.  [readability/naming/protected] [4]")
+        self.assert_lint('Ref<Node> protectedNode(otherNode);', "'protectedNode' is incorrectly named. It should be named 'protector' or 'protectedOtherNode'.  [readability/naming/protected] [4]")
+        self.assert_lint('Ref<Node> protectedNode(*otherNode);', "'protectedNode' is incorrectly named. It should be named 'protector' or 'protectedOtherNode'.  [readability/naming/protected] [4]")
+        self.assert_lint('RefPtr<Node> nodeRef(node);', "'nodeRef' is incorrectly named. It should be named 'protector' or 'protectedNode'.  [readability/naming/protected] [4]")
+        self.assert_lint('Ref<Node> nodeRef(*node);', "'nodeRef' is incorrectly named. It should be named 'protector' or 'protectedNode'.  [readability/naming/protected] [4]")
+
+        # Lines that look like a protector variable declaration but aren't.
+        self.assert_lint('static RefPtr<Widget> doSomethingWith(widget);', '')
+        self.assert_lint('RefPtr<Widget> create();', '')
+
     def test_parameter_names(self):
         # Leave meaningless variable names out of function declarations.

trunk/Websites/webkit.org/ChangeLog

@@ +1 @@
+2016-05-13  Brady Eidson  <beidson@apple.com>
+
+        Protector Ref/RefPtrs should have a specified naming style.
+        https://bugs.webkit.org/show_bug.cgi?id=157591
+
+        Reviewed by Darin Adler.
+
+        * code-style.md:
+
 2016-04-26  Timothy Hatcher  <timothy@apple.com>
 

trunk/Websites/webkit.org/code-style.md

@@ -712 +712 @@
 #ifndef _HTML_DOCUMENT_H_
 #define _HTML_DOCUMENT_H_
+```
+
+[](#names-protectors-this) Ref and RefPtr objects meant to protect `this` from deletion should be named "protectedThis".
+
+###### Right:
+
+```cpp
+RefPtr<Node> protectedThis(this);
+Ref<Element> protectedThis(*this);
+```
+
+###### Wrong:
+
+```cpp
+RefPtr<Node> protector(this);
+RefPtr<Widget> self(this);
+Ref<Element> elementRef(*this);
+```
+
+[](#names-protectors) Ref and RefPtr objects meant to protect variables other than `this` from deletion should be named either "protector", or "protected" combined with the capitalized form of the variable name.
+
+###### Right:
+
+```cpp
+RefPtr<Element> protector(&element);
+RefPtr<Node> protectedNode(node);
+RefPtr<Widget> protectedMainWidget(m_mainWidget);
+```
+
+###### Wrong:
+
+```cpp
+RefPtr<Node> nodeRef(&rootNode);
+Ref<Element> protect(*element);
+RefPtr<Widget> protected(widget);
+RefPtr<Node> protectorNode(node);
 ```