Summary: | check-webkit-style false positive for [readability/naming/protected] with method declaration | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
Component: | Tools / Tests | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | achristensen, beidson, bfulgham, darin, ews-watchlist, glenn, jbedard, useafterfree, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | Other | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=157591 https://bugs.webkit.org/show_bug.cgi?id=210680 |
||||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2020-04-17 20:19:46 PDT
Created attachment 396829 [details]
Patch v1
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. 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? 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! 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. Created attachment 398575 [details]
Patch v2
Comment on attachment 398575 [details]
Patch v2
I don’t understand the code, but I understand the test cases.
Committed r261415: <https://trac.webkit.org/changeset/261415> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398575 [details]. |