Skip to content

Commit

Permalink
Revise style checker guidelines for RefPtr variable names
Browse files Browse the repository at this point in the history 
https://bugs.webkit.org/show_bug.cgi?id=272485
rdar://126233155

Reviewed by Chris Dumez and David Kilzer.

Our current style guidelines discourage the use of the "protected" prefix or "protector" in new variable names when creating ref counted variables.
This updates the style checker to align with the current standard.

* Tools/Scripts/webkitpy/style/checkers/cpp.py:
(check_identifier_name_in_declaration):
* Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:
(WebKitStyleTest.test_names):

Canonical link: https://commits.webkit.org/277343@main
  • Loading branch information
@abigailfox
abigailfox committed Apr 11, 2024
1 parent cd75e44 commit 343bda4
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 44 deletions.
11 changes: 9 additions & 2 deletions Tools/Scripts/webkitpy/style/checkers/cpp.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4082,15 +4082,22 @@ def check_identifier_name_in_declaration(filename, line_number, line, file_state
protector_name = ref_check.group('protector_name')
protected_name = ref_check.group('protected_name')
cap_protected_name = protected_name[0].upper() + protected_name[1:]

# Ignore function declarations where cap_protected_name == protected_name indicates a type name.
if cap_protected_name != protected_name:
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':
elif protected_name == 'this' and protector_name == 'protectedThis':
return
elif protected_name != 'this' and protector_name == 'protectedThis':
error(line_number, 'readability/naming/protected', 4, "Incorrect use of \'protectedThis\'.")
elif protector_name.startswith("protected"):
error(line_number, 'readability/naming/protected', 4, "\'" + protector_name + "\' is incorrectly named. New variables should not use the \'protected\' prefix.")
elif protector_name == 'protector':
error(line_number, 'readability/naming/protected', 4, "\'" + protector_name + "\' is incorrectly named. New variables should not be named \'protector\'")
else:
error(line_number, 'readability/naming/protected', 4, "\'" + protector_name + "\' is incorrectly named. It should be named \'protector\' or \'" + expected_protector_name + "\'.")
return

# Basically, a declaration is a type name followed by whitespaces
# followed by an identifier. The type name can be complicated
Expand Down
90 changes: 48 additions & 42 deletions Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6276,66 +6276,72 @@ def test_names(self):
# 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);', '')
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> nodeRef(node);', '')
self.assert_lint('Ref<Node> nodeRef(*node);', '')
self.assert_lint('Ref<Node> node(*node);', '')
self.assert_lint('Ref<Node> nodeRef = node;', '')

# Invalid protector RefPtr/Ref variable names.
self.assert_lint('RefPtr<Node> protectedNode(node);', "'protectedNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Node> protectedNode(&node);', "'protectedNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Node> protector(node);', "'protector' is incorrectly named. New variables should not be named 'protector' [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Node> protector(&node);', "'protector' is incorrectly named. New variables should not be named 'protector' [readability/naming/protected] [4]")
self.assert_lint('Ref<Node> protectedNode(node);', "'protectedNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('Ref<Node> protectedNode(*node);', "'protectedNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('Ref<Node> protector(node);', "'protector' is incorrectly named. New variables should not be named 'protector' [readability/naming/protected] [4]")
self.assert_lint('Ref<Node> protector(*node);', "'protector' is incorrectly named. New variables should not be named 'protector' [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Node> protectedOtherNode(otherNode);', "'protectedOtherNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Node> protectedOtherNode(&otherNode);', "'protectedOtherNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('Ref<Node> protectedOtherNode(otherNode);', "'protectedOtherNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('Ref<Node> protectedOtherNode(*otherNode);', "'protectedOtherNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Widget> protectedWidget(m_widget);', "'protectedWidget' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Widget> protectedWidget(&m_widget);', "'protectedWidget' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('Ref<Widget> protectedWidget(m_widget);', "'protectedWidget' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('Ref<Widget> protectedWidget(*m_widget);', "'protectedWidget' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Widget> protector(m_widget);', "'protector' is incorrectly named. New variables should not be named 'protector' [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Widget> protector(&m_widget);', "'protector' is incorrectly named. New variables should not be named 'protector' [readability/naming/protected] [4]")
self.assert_lint('Ref<Widget> protector(m_widget);', "'protector' is incorrectly named. New variables should not be named 'protector' [readability/naming/protected] [4]")
self.assert_lint('Ref<Widget> protector(*m_widget);', "'protector' is incorrectly named. New variables should not be named 'protector' [readability/naming/protected] [4]")
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> 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> 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]")
self.assert_lint('RefPtr<Node> protectedThis(node);', "Incorrect use of 'protectedThis'. [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Node> protectedThis(&node);', "Incorrect use of 'protectedThis'. [readability/naming/protected] [4]")
self.assert_lint('Ref<Node> protectedThis(node);', "Incorrect use of 'protectedThis'. [readability/naming/protected] [4]")
self.assert_lint('Ref<Node> protectedThis(*node);', "Incorrect use of 'protectedThis'. [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Node> protectedNode(otherNode);', "'protectedNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Node> protectedNode(&otherNode);', "'protectedNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('Ref<Node> protectedNode(otherNode);', "'protectedNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('Ref<Node> protectedNode(*otherNode);', "'protectedNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Node> protectedNode = otherNode;', "'protectedNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Node> protectedNode = &otherNode;', "'protectedNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('Ref<Node> protectedNode = otherNode;', "'protectedNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('Ref<Node> protectedNode = *otherNode;', "'protectedNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Node> protectedNode = node;', "'protectedNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Node> protectedNode = &node;', "'protectedNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Node> protector = node;', "'protector' is incorrectly named. New variables should not be named 'protector' [readability/naming/protected] [4]")
self.assert_lint('RefPtr<Node> protector = &node;', "'protector' is incorrectly named. New variables should not be named 'protector' [readability/naming/protected] [4]")
self.assert_lint('Ref<Node> protectedNode = node;', "'protectedNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('Ref<Node> protectedNode = *node;', "'protectedNode' is incorrectly named. New variables should not use the 'protected' prefix. [readability/naming/protected] [4]")
self.assert_lint('Ref<Node> protector = node;', "'protector' is incorrectly named. New variables should not be named 'protector' [readability/naming/protected] [4]")
self.assert_lint('Ref<Node> protector = *node;', "'protector' is incorrectly named. New variables should not be named 'protector' [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();', '')
self.assert_lint('Ref<GeolocationPermissionRequestProxy> createRequest(GeolocationIdentifier);', '')
self.assert_lint('Ref<TypeName> createSomething(OtherTypeName);', '')
self.assert_lint('Ref<Settings> protectedSettings() const;', '')
self.assert_lint('RefPtr<Settings> protectedSettings() const;', '')
self.assert_lint('RefPtr<Settings> protectedSettings();', '')
self.assert_lint('RefPtr<Settings> protectedSettings() { return m_settings.get(); }', '')

def test_parameter_names(self):
# Leave meaningless variable names out of function declarations.
Expand Down

0 comments on commit 343bda4

Please sign in to comment.