After fixing Bug 214954, I thought of a better algorithm to check for acronym capitalization in an identifier.
Created attachment 405729 [details] Patch v1
Comment on attachment 405729 [details] Patch v1 Oops, need to fix a test outside of the ones I was working on.
Created attachment 405733 [details] Patch v2
(In reply to David Kilzer (:ddkilzer) from comment #2) > Comment on attachment 405729 [details] > Patch v1 > > Oops, need to fix a test outside of the ones I was working on. LOL! These changes found a typo in an unrelated test! self.assert_multi_line_lint( - 'auto Foo:bar() -> Baz\n' + 'auto Foo::bar() -> Baz\n' '{\n' '}\n', '')
Comment on attachment 405733 [details] Patch v2 How well do "possible error"/"maybe" messages fit into how we use check-webkit-style?
(In reply to Darin Adler from comment #5) > Comment on attachment 405733 [details] > Patch v2 > > How well do "possible error"/"maybe" messages fit into how we use > check-webkit-style? We might want an allow-list so that we can optionally override common ones, but it's relatively common to ignore check-webkit-style errors if the engineers knows better.
(In reply to David Kilzer (:ddkilzer) from comment #3) > Created attachment 405733 [details] > Patch v2 I can't reproduce webkitpy test failures: $ ./Tools/Scripts/test-webkitpy webkitpy.style.checkers.cpp_unittest Suppressing most webkitpy logging while running unit tests. Ran 209 tests in 0.585s OK $
(In reply to David Kilzer (:ddkilzer) from comment #7) > (In reply to David Kilzer (:ddkilzer) from comment #3) > > Created attachment 405733 [details] > > Patch v2 > > I can't reproduce webkitpy test failures: > > $ ./Tools/Scripts/test-webkitpy webkitpy.style.checkers.cpp_unittest > Suppressing most webkitpy logging while running unit tests. > Ran 209 tests in 0.585s > > > OK > $ Oh! All of the failures are on Python3?! > 1:03 Passed webkitpy python2 tests, Found 94 webkitpy python3 test failures:
(In reply to David Kilzer (:ddkilzer) from comment #8) > (In reply to David Kilzer (:ddkilzer) from comment #7) > > (In reply to David Kilzer (:ddkilzer) from comment #3) > > > Created attachment 405733 [details] > > > Patch v2 > > > > I can't reproduce webkitpy test failures: > > > > $ ./Tools/Scripts/test-webkitpy webkitpy.style.checkers.cpp_unittest > > Suppressing most webkitpy logging while running unit tests. > > Ran 209 tests in 0.585s > > > > > > OK > > $ > > Oh! All of the failures are on Python3?! > > > 1:03 Passed webkitpy python2 tests, Found 94 webkitpy python3 test failures: xrange() failure.
(In reply to David Kilzer (:ddkilzer) from comment #9) > (In reply to David Kilzer (:ddkilzer) from comment #8) > > (In reply to David Kilzer (:ddkilzer) from comment #7) > > > (In reply to David Kilzer (:ddkilzer) from comment #3) > > > > Created attachment 405733 [details] > > > > Patch v2 > > > > > > I can't reproduce webkitpy test failures: > > > > > > $ ./Tools/Scripts/test-webkitpy webkitpy.style.checkers.cpp_unittest > > > Suppressing most webkitpy logging while running unit tests. > > > Ran 209 tests in 0.585s > > > > > > > > > OK > > > $ > > > > Oh! All of the failures are on Python3?! > > > > > 1:03 Passed webkitpy python2 tests, Found 94 webkitpy python3 test failures: > > xrange() failure. You can just use range(...) Small efficiency hit on Python 2, but that doesn't really matter with lists as short as the ones we're dealing with here.
(In reply to Jonathan Bedard from comment #6) > (In reply to Darin Adler from comment #5) > > Comment on attachment 405733 [details] > > Patch v2 > > > > How well do "possible error"/"maybe" messages fit into how we use > > check-webkit-style? > > We might want an allow-list so that we can optionally override common ones, > but it's relatively common to ignore check-webkit-style errors if the > engineers knows better. Here are the lower-confidence '[3]' issues found in Source/WebKit: ERROR: Source/WebKit/UIProcess/API/C/WKDownload.cpp:71: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebKit/UIProcess/ios/forms/WKFileUploadPanel.mm:630: The identifier name "urlsCount" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] And in Source/WebCore (excluding Source/WebCore/plugins/npfunctions.h): ERROR: Source/WebCore/svg/SVGURIReference.cpp:72: The identifier name "kurl" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/svg/SVGScriptElement.cpp:82: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/svg/SVGFEImageElement.cpp:192: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/svg/SVGCursorElement.cpp:99: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/svg/SVGImageElement.cpp:163: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/inspector/agents/InspectorPageAgent.cpp:721: The identifier name "kurl" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/plugins/PluginData.h:67: The identifier name "mimes" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/platform/win/PasteboardWin.cpp:617: The identifier name "kurl" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/platform/win/PasteboardWin.cpp:691: The identifier name "kurl" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/platform/mediastream/MediaEndpointConfiguration.cpp:49: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/platform/mediastream/MediaEndpointConfiguration.h:48: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateAVFoundationObjC.mm:393: The identifier name "urlsToDelete" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/html/HTMLParamElement.cpp:71: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/html/HTMLImageElement.cpp:728: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/html/HTMLBodyElement.cpp:222: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/html/HTMLInputElement.cpp:1596: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/html/HTMLTableElement.cpp:578: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/html/HTMLScriptElement.cpp:118: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/html/HTMLStyleElement.cpp:146: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/html/HTMLTableCellElement.cpp:195: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/html/HTMLLinkElement.cpp:617: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/html/HTMLEmbedElement.cpp:221: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] ERROR: Source/WebCore/html/HTMLObjectElement.cpp:458: The identifier name "urls" _may_ contain an acronym that is not all uppercase. [readability/naming/acronym] [3] I could even add 'urls' to the exception list if that seems okay. That would leave one 'mimes' variable and four 'kurl' variables (from a by-gone era when we used to have a KURL class). There are no [3] issues in Source/WebKitLegacy or Source/JavaScriptCore.
Created attachment 405766 [details] Patch v3
Comment on attachment 405766 [details] Patch v3 Looks OK. We should stay on the lookout for false positives in the wild as people write new code.
Created attachment 406100 [details] Patch for landing
(In reply to David Kilzer (:ddkilzer) from comment #14) > Created attachment 406100 [details] > Patch for landing Added 'urls' to the acronym_exception list.
Comment on attachment 406100 [details] Patch for landing Marking cq+ since this is a Tools-only change and webkitpy tests passed.
Committed r265343: <https://trac.webkit.org/changeset/265343> All reviewed patches have been landed. Closing bug and clearing flags on attachment 406100 [details].
<rdar://problem/66641418>