Bug 131691

<rdar://problem/16625128>

Created attachment 229841 [details] Patch

Attachment 229841 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] Total errors found: 4 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 229847 [details] Patch

Attachment 229847 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] Total errors found: 4 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 229847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229847&action=review > Source/JavaScriptCore/API/APIData.h:36 > +class APIData { Maybe this can just be a struct rather than having getters and setters. > Source/JavaScriptCore/API/APIData.h:40 > + APIData() > + { > + } Should just omit this. Generated one should be fine. > Source/JavaScriptCore/API/APIData.h:62 > + RetainPtr<CFTypeRef> m_externalObjectGraph; > + RetainPtr<CFTypeRef> m_externalRememberedSet; Why so coy about the fact that the type is NSMapTable? Is there a problem compiling that when it’s non-Objective-C++? > Source/JavaScriptCore/API/JSVirtualMachine.mm:118 > + vm.apiData.setExternalObjectGraph(RetainPtr<CFTypeRef>( > + [[NSMapTable alloc] initWithKeyOptions:weakIDOptions valueOptions:strongIDOptions capacity:0])); This leaks. It needs to be adoptNS, not just a conversion to RetainPtr. > Source/JavaScriptCore/API/JSVirtualMachine.mm:125 > + vm.apiData.setExternalRememberedSet(RetainPtr<CFTypeRef>( > + [[NSMapTable alloc] initWithKeyOptions:weakIDOptions valueOptions:integerOptions capacity:0])); Ditto. > Source/JavaScriptCore/API/JSVirtualMachine.mm:187 > + NSMapTable *externalObjectGraph = static_cast<id>(vm.apiData.externalObjectGraph()); Why is this one casting to <id>, but all the others casting to NSMapTable? > Source/JavaScriptCore/runtime/VM.h:213 > + APIData apiData; Why so high in the class? Is this in some sort of order?

Created attachment 229854 [details] Fix build

Attachment 229854 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] Total errors found: 4 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 229856 [details] Fix build

Attachment 229856 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] Total errors found: 4 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 229887 [details] Fix build

Attachment 229887 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/APIData.h:26: #ifndef header guard has wrong style, please use: APIData_h [build/header_guard] [5] Total errors found: 5 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 229890 [details] Fix build

Attachment 229890 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/APIData.h:26: #ifndef header guard has wrong style, please use: APIData_h [build/header_guard] [5] Total errors found: 5 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 229918 [details] Fix build

Attachment 229918 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/APIData.h:26: #ifndef header guard has wrong style, please use: APIData_h [build/header_guard] [5] Total errors found: 5 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 229933 [details] I have a dream

Attachment 229933 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/APIData.h:26: #ifndef header guard has wrong style, please use: APIData_h [build/header_guard] [5] Total errors found: 5 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 229941 [details] that one day

Created attachment 229943 [details] we will live in a world

Created attachment 229944 [details] where a patch

Attachment 229944 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:161: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:170: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:193: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/tests/MemoryManagementTests.mm:196: Multi-line string ("...") found. This lint script doesn't do well with such strings, and may give bogus warnings. They're ugly and unnecessary, and you should use concatenation instead". [readability/multiline_string] [5] ERROR: Source/JavaScriptCore/API/APIData.h:26: #ifndef header guard has wrong style, please use: APIData_h [build/header_guard] [5] Total errors found: 5 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 229946 [details] will be judged

Comment on attachment 229946 [details] will be judged is this patch still relevant?

Comment on attachment 229946 [details] will be judged Hi, Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting. To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.