Bug 210682 – check-webkit-style false positive for [readability/naming/protected] with method declaration

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

check-webkit-style false positive for [readability/naming/protected] with method declaration.

    Ref<GeolocationPermissionRequestProxy> createRequest(GeolocationIdentifier);

ERROR: Source/WebKit/UIProcess/GeolocationPermissionRequestManagerProxy.h:46:  'createRequest' is incorrectly named. It should be named 'protector' or 'protectedGeolocationIdentifier'.  [readability/naming/protected] [4]

[David Kilzer (:ddkilzer)]

Created attachment 396829 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Comment on attachment 396829 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=396829&action=review

> Tools/Scripts/webkitpy/style/checkers/cpp.py:3676
> +        # Ignore function declarations where cap_protected_name == protected_name indicates a type name.
> +        if not cap_protected_name == protected_name:

I considered checking whether `protector_name` started with 'create' instead, but I thought the capitalization check would likely result in better results based on our rules for starting variables with lowercase letters.

[Jonathan Bedard]

Comment on attachment 396829 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=396829&action=review

>> Tools/Scripts/webkitpy/style/checkers/cpp.py:3676
>> +        if not cap_protected_name == protected_name:
> 
> I considered checking whether `protector_name` started with 'create' instead, but I thought the capitalization check would likely result in better results based on our rules for starting variables with lowercase letters.

Feel like we would usually use != instead of not *** == ***.

> Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:5787
> +        self.assert_lint('Ref<GeolocationPermissionRequestProxy> createRequest(GeolocationIdentifier);', '')

Could we have a counter-example? Ie, a line where cap_protected_name == protected_name and we don't lint?

[David Kilzer (:ddkilzer)]

Comment on attachment 396829 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=396829&action=review

>>> Tools/Scripts/webkitpy/style/checkers/cpp.py:3676
>>> +        if not cap_protected_name == protected_name:
>> 
>> I considered checking whether `protector_name` started with 'create' instead, but I thought the capitalization check would likely result in better results based on our rules for starting variables with lowercase letters.
> 
> Feel like we would usually use != instead of not *** == ***.

Will fix.

>> Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:5787
>> +        self.assert_lint('Ref<GeolocationPermissionRequestProxy> createRequest(GeolocationIdentifier);', '')
> 
> Could we have a counter-example? Ie, a line where cap_protected_name == protected_name and we don't lint?

The counter-example should never exist because we wouldn't name a variable that started with a capital letter!

[David Kilzer (:ddkilzer)]

Comment on attachment 396829 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=396829&action=review

>>> Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:5787
>>> +        self.assert_lint('Ref<GeolocationPermissionRequestProxy> createRequest(GeolocationIdentifier);', '')
>> 
>> Could we have a counter-example? Ie, a line where cap_protected_name == protected_name and we don't lint?
> 
> The counter-example should never exist because we wouldn't name a variable that started with a capital letter!

Please disregard this comment.  I understand what you meant now.  Will add.

[David Kilzer (:ddkilzer)]

Created attachment 398575 [details]
Patch v2

[Darin Adler]

Comment on attachment 398575 [details]
Patch v2

I don’t understand the code, but I understand the test cases.

[EWS]

Committed r261415: <https://trac.webkit.org/changeset/261415>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398575 [details].

[Radar WebKit Bug Importer]

<rdar://problem/63035760>