Bug 215026 – check-webkit-style: better algorithm to check for acronym capitalization in an identifier

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

After fixing Bug 214954, I thought of a better algorithm to check for acronym capitalization in an identifier.

[David Kilzer (:ddkilzer)]

Created attachment 405729 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Comment on attachment 405729 [details]
Patch v1

Oops, need to fix a test outside of the ones I was working on.

[David Kilzer (:ddkilzer)]

Created attachment 405733 [details]
Patch v2

[David Kilzer (:ddkilzer)]

(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',
             '')

[Darin Adler]

Comment on attachment 405733 [details]
Patch v2

How well do "possible error"/"maybe" messages fit into how we use check-webkit-style?

[Jonathan Bedard]

(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.

[David Kilzer (:ddkilzer)]

(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
$

[David Kilzer (:ddkilzer)]

(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:

[David Kilzer (:ddkilzer)]

(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.

[Jonathan Bedard]

(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.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

Created attachment 405766 [details]
Patch v3

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

Created attachment 406100 [details]
Patch for landing

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #14)
> Created attachment 406100 [details]
> Patch for landing

Added 'urls' to the acronym_exception list.

[David Kilzer (:ddkilzer)]

Comment on attachment 406100 [details]
Patch for landing

Marking cq+ since this is a Tools-only change and webkitpy tests passed.

[EWS]

Committed r265343: <https://trac.webkit.org/changeset/265343>

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

[Radar WebKit Bug Importer]

<rdar://problem/66641418>