Bug 171486 - [Cocoa] Have check-webkit-style advise against use of [get…Class() alloc]
Summary: [Cocoa] Have check-webkit-style advise against use of [get…Class() alloc]
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: mitz
URL:
Keywords:
Depends on:
Blocks: 171493 171514
  Show dependency treegraph
 
Reported: 2017-04-30 14:46 PDT by mitz
Modified: 2017-05-01 14:32 PDT (History)
8 users (show)

See Also:


Attachments
Add style check (2.28 KB, patch)
2017-04-30 14:49 PDT, mitz
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2017-04-30 14:46:52 PDT
It’s safer to use alloc…Instance() so the style checker could suggest that.

Patch forthcoming.
Comment 1 mitz 2017-04-30 14:49:57 PDT
Created attachment 308701 [details]
Add style check
Comment 2 mitz 2017-04-30 16:40:04 PDT
Committed <https://trac.webkit.org/r216000>.
Comment 3 Daniel Bates 2017-04-30 17:11:22 PDT
Can we add a unit test for this change? Notice that we have unit tests in <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py>. To run the unit tests, run Tools/Scripts/test-webkitpy.
Comment 4 David Kilzer (:ddkilzer) 2017-05-01 06:11:28 PDT
The webkitpy unit test started to fail with this change:

[1068/1576] webkitpy.style.checkers.cpp_unittest.CppStyleTest.test_spacing_before_brackets failed:                      
  Traceback (most recent call last):
    File "./Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 1885, in test_spacing_before_brackets
      self.assert_lint('        return [get_##framework##_##className##Class() alloc]; \\', '')
    File "./Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 317, in assert_lint
      self.assertEqual(expected_message, self.perform_single_line_lint(code, file_name))
    File "./Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 270, in perform_single_line_lint
      return self.perform_lint(code, filename, basic_error_rules)
    File "./Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 259, in perform_lint
      self.process_file_data(filename, extension, lines, error_collector, unit_test_config)
    File "./Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 253, in process_file_data
      checker.check(lines)
    File "./Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3986, in check
      self.handle_style_error, self.min_confidence)
    File "./Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3846, in _process_lines
      enum_state, asm_state, error)
    File "./Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3791, in process_line
      check_style(clean_lines, line, file_extension, class_state, file_state, enum_state, error)
    File "./Tools/Scripts/webkitpy/style/checkers/cpp.py", line 2842, in check_style
      check_soft_link_class_alloc(clean_lines, line_number, error)
    File "./Tools/Scripts/webkitpy/style/checkers/cpp.py", line 2746, in check_soft_link_class_alloc
      'Using +alloc with a soft-linked class. Use alloc%sInstance() instead.' % matched.group(1))
    File "./Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 69, in __call__
      ' which is not in CppChecker.categories' % (message, category))
  AssertionError: Message "Using +alloc with a soft-linked class. Use alloc_##framework##_##className##Instance() instead." has category "runtime/soft-linked-alloc", which is not in CppChecker.categories
  
Ran 1576 tests in 5.895s                                                                                                
FAILED (failures=1, errors=0)

And fixing that results in a false-positive match for the macro:

[1056/1576] webkitpy.style.checkers.cpp_unittest.CppStyleTest.test_spacing_before_brackets failed:                      
  Traceback (most recent call last):
    File "./Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 1885, in test_spacing_before_brackets
      self.assert_lint('        return [get_##framework##_##className##Class() alloc]; \\', '')
    File "./Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py", line 317, in assert_lint
      self.assertEqual(expected_message, self.perform_single_line_lint(code, file_name))
  AssertionError: '' != 'Using +alloc with a soft-linked class. Use alloc_##framework##_##className##Instance() instead.  [runtime/soft-linked-alloc] [4]'
  
Ran 1576 tests in 5.756s                                                                                                
FAILED (failures=1, errors=0)

Fixed in r216013:  <https://trac.webkit.org/r216013>
Comment 5 David Kilzer (:ddkilzer) 2017-05-01 06:26:46 PDT
And FWIW, TestWebKitAPI has a few hits left:

$ ./Tools/Scripts/check-webkit-style --filter=-,+runtime/soft-linked-alloc Tools/TestWebKitAPI
ERROR: Tools/TestWebKitAPI/Tests/WebKit/ios/AudioSessionCategoryIOS.mm:69:  Using +alloc with a soft-linked class. Use allocUIWindowInstance() instead.  [runtime/soft-linked-alloc] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit/ios/AudioSessionCategoryIOS.mm:70:  Using +alloc with a soft-linked class. Use allocUIWebViewInstance() instead.  [runtime/soft-linked-alloc] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2Cocoa/RequiresUserActionForPlayback.mm:76:  Using +alloc with a soft-linked class. Use allocUIWindowInstance() instead.  [runtime/soft-linked-alloc] [4]
Total errors found: 3 in 755 files

Filed: Bug 171493: [Cocoa] Replace uses of [get…Class() alloc] with alloc…Instance() in TestWebKitAPI
<https://bugs.webkit.org/show_bug.cgi?id=171493>
Comment 6 David Kilzer (:ddkilzer) 2017-05-01 14:32:54 PDT
And Source/WebCore has a couple left, too:

$ ./Tools/Scripts/check-webkit-style --filter=-,+runtime/soft-linked-alloc Source/WebCore
ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:447:  Using +alloc with a soft-linked class. Use allocWebAVPictureInPicturePlayerLayerViewInstance() instead.  [runtime/soft-linked-alloc] [4]
ERROR: Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:616:  Using +alloc with a soft-linked class. Use allocWebAVPlayerLayerViewInstance() instead.  [runtime/soft-linked-alloc] [4]
Total errors found: 2 in 8492 files

Filed: Bug 171514: [Cocoa] Replace uses of [get…Class() alloc] in Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm