Bug 198604

Summary: Refactoring of architectural Register Information
Product: WebKit Reporter: Paulo Matos <pmatos>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: chris.reid, commit-queue, darin, don.olmstead, ews-watchlist, guijemont, keith_miller, mark.lam, msaboff, saam, ticaiolima, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews211 for win-future
none
Revised Patch
none
Revised Patch
none
Revised Patch
none
Revised Patch
none
Revised Patch
none
Revised Patch
none
Post-1 Review Patch
none
Post-1 Review Patch
none
Post-1 Review Patch
none
Post-1 Review Patch
saam: review-
Post-2 Review Patch
none
Post-3 Review Patch
none
Post-3 Review Patch
none
Post-4 Review Patch
none
Post-5 Review Patch
keith_miller: review-
Post-6 Review Patch none

Paulo Matos
Reported 2019-06-06 03:34:25 PDT
Created attachment 371485 [details] Patch This patch adds a new register information file per architecture <arch>Registers.h, which is then used by <arch>Assembler.cpp and RegisterSet.cpp. The goal here is to have all the register information in a single place - the <arch>Registers.h tables, instead of spread accross different files in the source code.
Attachments
Patch (54.83 KB, patch)
2019-06-06 03:34 PDT, Paulo Matos 		ews-watchlist: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from ews211 for win-future (13.92 MB, application/zip)
2019-06-06 08:29 PDT, EWS Watchlist 		no flags
Details
Revised Patch (58.26 KB, patch)
2019-06-06 14:00 PDT, Paulo Matos 		no flags
DetailsFormatted DiffDiff
Revised Patch (58.21 KB, patch)
2019-06-06 14:30 PDT, Paulo Matos 		no flags
DetailsFormatted DiffDiff
Revised Patch (58.21 KB, patch)
2019-06-07 05:26 PDT, Paulo Matos 		no flags
DetailsFormatted DiffDiff
Revised Patch (72.44 KB, patch)
2019-06-10 00:31 PDT, Paulo Matos 		no flags
DetailsFormatted DiffDiff
Revised Patch (72.44 KB, patch)
2019-06-10 01:31 PDT, Paulo Matos 		no flags
DetailsFormatted DiffDiff
Revised Patch (72.78 KB, patch)
2019-06-10 04:24 PDT, Paulo Matos 		no flags
DetailsFormatted DiffDiff
Post-1 Review Patch (74.22 KB, patch)
2019-06-12 03:23 PDT, Paulo Matos 		no flags
DetailsFormatted DiffDiff
Post-1 Review Patch (74.22 KB, patch)
2019-06-12 05:12 PDT, Paulo Matos 		no flags
DetailsFormatted DiffDiff
Post-1 Review Patch (74.36 KB, patch)
2019-06-12 05:17 PDT, Paulo Matos 		no flags
DetailsFormatted DiffDiff
Post-1 Review Patch (74.42 KB, patch)
2019-06-12 07:54 PDT, Paulo Matos 		saam: review-
DetailsFormatted DiffDiff
Post-2 Review Patch (77.49 KB, patch)
2019-06-13 00:53 PDT, Paulo Matos 		no flags
DetailsFormatted DiffDiff
Post-3 Review Patch (75.65 KB, patch)
2019-06-13 07:10 PDT, Paulo Matos 		no flags
DetailsFormatted DiffDiff
Post-3 Review Patch (75.65 KB, patch)
2019-06-13 07:19 PDT, Paulo Matos 		no flags
DetailsFormatted DiffDiff
Post-4 Review Patch (75.51 KB, patch)
2019-06-26 03:56 PDT, Paulo Matos 		no flags
DetailsFormatted DiffDiff
Post-5 Review Patch (75.51 KB, patch)
2019-06-27 00:54 PDT, Paulo Matos 		keith_miller: review-
DetailsFormatted DiffDiff
Post-6 Review Patch (75.58 KB, patch)
2019-07-03 00:16 PDT, Paulo Matos 		no flags
DetailsFormatted DiffDiff
Show Obsolete (17) View All Add attachment proposed patch, testcase, etc.
EWS Watchlist
Comment 1 2019-06-06 03:39:45 PDT
Attachment 371485 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/assembler/X86Assembler.h:37: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/JavaScriptCore/assembler/X86Assembler.h:56: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/X86Assembler.h:57: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/X86Assembler.h:61: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/X86Assembler.h:65: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/X86Assembler.h:66: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/RegisterInfo.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] ERROR: Source/JavaScriptCore/assembler/MIPSAssembler.h:38: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/JavaScriptCore/assembler/ARM64Assembler.h:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/assembler/ARM64Assembler.h:176: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Assembler.h:197: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/RegisterSet.cpp:55: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/jit/RegisterSet.cpp:113: Missing space before ( in if( [whitespace/parens] [5] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:45: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:54: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:56: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:64: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:67: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:69: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:80: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:86: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:95: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:97: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:105: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:108: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:110: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:121: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:140: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:149: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:158: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARMv7Assembler.h:33: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 31 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Paulo Matos
Comment 2 2019-06-06 04:03:15 PDT
Some of the complains are false positives. Reported that issue separately: https://bugs.webkit.org/show_bug.cgi?id=198605
EWS Watchlist
Comment 3 2019-06-06 08:29:22 PDT
Comment on attachment 371485 [details] Patch Attachment 371485 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12394781 New failing tests: imported/blink/fast/canvas/canvas-clip-stack-persistence.html css3/filters/blur-various-radii.html
EWS Watchlist
Comment 4 2019-06-06 08:29:25 PDT
Created attachment 371498 [details] Archive of layout-test-results from ews211 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews211 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Paulo Matos
Comment 5 2019-06-06 14:00:12 PDT
Created attachment 371525 [details] Revised Patch
EWS Watchlist
Comment 6 2019-06-06 14:02:53 PDT
Attachment 371525 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/CMakeLists.txt:465: No trailing spaces [whitespace/trailing] [5] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:45: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:54: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:56: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:64: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:67: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:69: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:80: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:86: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:95: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:97: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:105: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:108: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:110: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:121: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:140: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:149: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:158: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 18 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Paulo Matos
Comment 7 2019-06-06 14:30:18 PDT
Created attachment 371526 [details] Revised Patch
EWS Watchlist
Comment 8 2019-06-06 14:32:12 PDT
Attachment 371526 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/RegisterSet.cpp:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:45: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:54: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:56: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:64: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:67: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:69: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:80: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:86: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:95: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:97: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:105: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:108: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:110: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:121: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:140: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:149: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:158: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 18 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Paulo Matos
Comment 9 2019-06-07 05:26:57 PDT
Created attachment 371583 [details] Revised Patch
EWS Watchlist
Comment 10 2019-06-07 05:29:14 PDT
Attachment 371583 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/RegisterSet.cpp:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:45: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:54: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:56: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:64: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:67: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:69: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:80: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:86: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:95: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:97: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:105: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:108: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:110: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:121: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:140: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:149: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:158: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 18 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Paulo Matos
Comment 11 2019-06-10 00:31:11 PDT
Created attachment 371727 [details] Revised Patch
EWS Watchlist
Comment 12 2019-06-10 00:34:07 PDT
Attachment 371727 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/RegisterSet.cpp:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:45: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:54: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:56: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:64: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:67: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:69: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:80: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:86: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:95: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:97: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:105: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:108: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:110: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:121: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:140: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:149: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:158: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 18 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Paulo Matos
Comment 13 2019-06-10 01:31:02 PDT
Created attachment 371729 [details] Revised Patch
EWS Watchlist
Comment 14 2019-06-10 01:33:14 PDT
Attachment 371729 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/RegisterSet.cpp:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:45: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:54: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:56: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:64: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:67: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:69: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:80: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:86: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:95: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:97: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:105: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:108: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:110: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:121: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:140: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:149: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:158: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 18 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Paulo Matos
Comment 15 2019-06-10 04:24:56 PDT
Created attachment 371733 [details] Revised Patch
EWS Watchlist
Comment 16 2019-06-10 04:26:38 PDT
Attachment 371733 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/jit/RegisterSet.cpp:31: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:45: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:54: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:56: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:64: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:67: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:69: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:80: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:86: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:95: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:97: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:105: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:108: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:110: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:121: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:140: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:149: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:158: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 18 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Paulo Matos
Comment 17 2019-06-11 00:01:51 PDT
What are the architectural differences between win and wincairo which could make win pass and wincairo fail? I am unable to reproduce it on a local windows machine. Therefore I am marking the patch for review. Style problems are due to false positives reported in: https://bugs.webkit.org/show_bug.cgi?id=198605
Caio Lima
Comment 18 2019-06-11 10:24:42 PDT
Comment on attachment 371733 [details] Revised Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371733&action=review I didn't read everything, but I've left some comments. > Source/JavaScriptCore/assembler/ARM64Assembler.h:169 > +// in ARM64Registers.h on the macro definition of REGNS nit: There is some extra space here. > Source/JavaScriptCore/assembler/ARM64Assembler.h:173 > +#define REGISTER_ID(id, nsid, name, r, cs) id, nit: Since we are not using `r` and `cs`, I think we could remove them. > Source/JavaScriptCore/assembler/ARM64Assembler.h:185 > +#define REGISTER_ID(id, nsid, name) id, ditto. > Source/JavaScriptCore/assembler/ARM64Assembler.h:194 > +#define REGISTER_ID(id, nsid, name, r, cs) id, Ditto. > Source/JavaScriptCore/assembler/ARM64Assembler.h:229 > +#define REGISTER_NAME(id, nsid, name, r, cs) name, Ditto. > Source/JavaScriptCore/assembler/ARM64Assembler.h:251 > +#define REGISTER_NAME(id, nsid, name, r, cs) name, Ditto. > Source/JavaScriptCore/assembler/ARM64Registers.h:29 > +#define REGNS ARM64Registers One suggestion is to use `REGISTER_NAMESPACE` instead of `REGNS`. > Source/JavaScriptCore/assembler/ARM64Registers.h:36 > +#define macro(m, ...) m(__VA_ARGS__) Over JSC codebase we usually have `FOR_EACH...(macro)` style. Using `macro` seems a little bit confusing to me. Can we rename it to `macro_forward` or something else?
Caio Lima
Comment 19 2019-06-11 11:04:12 PDT
Comment on attachment 371733 [details] Revised Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371733&action=review LGTM. I left more comments and we also need to figure out why wincario is not building. >> Source/JavaScriptCore/assembler/ARM64Assembler.h:173 >> +#define REGISTER_ID(id, nsid, name, r, cs) id, > > nit: Since we are not using `r` and `cs`, I think we could remove them. Never mind this comment. We need to have those. > Source/JavaScriptCore/assembler/MIPSAssembler.h:108 > +#undef REGISTER_NAME Nice! > Source/JavaScriptCore/assembler/MIPSRegisters.h:108 > + macro(m, nn(fir), 0) \ I think it would be very good have an commentary documenting each param here, so we don't need to go to files where this `FOR_EACH` is used to figure out what each value means. > Source/JavaScriptCore/jit/RegisterSet.cpp:51 > +#define SET_IF_RESERVED(id, nsid, name, r, cs) if (r) result.set(nsid); I think we should rename `r` to `isReserved`. > Source/JavaScriptCore/jit/RegisterSet.cpp:113 > +#define SET_IF_CALLEESAVED(id, nsid, name, r, cs) if (cs) result.set(nsid); I think it would be better rename `cs` to `isCalleeSaved`.
Don Olmstead
Comment 20 2019-06-11 16:11:35 PDT
(In reply to Paulo Matos from comment #17) > What are the architectural differences between win and wincairo which could > make win pass and wincairo fail? I am unable to reproduce it on a local > windows machine. Therefore I am marking the patch for review. Style problems > are due to false positives reported in: > https://bugs.webkit.org/show_bug.cgi?id=198605 It appears we're hitting differences between the preprocessor with MSVC and Clang/GCC. Chris is working out a fix and it looks like technically MSVC has an experimental preprocessor, https://devblogs.microsoft.com/cppblog/msvc-preprocessor-progress-towards-conformance/ , without this issue. Anyways if you can wait to see if Chris can get a version without the issue that would be appreciated.
Christopher Reid
Comment 21 2019-06-11 19:23:06 PDT
Comment on attachment 371733 [details] Revised Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371733&action=review > Source/JavaScriptCore/assembler/X86_64Registers.h:33 > +#define macro(m, ...) m(__VA_ARGS__) The wincairo build is failing because MSVC expands the m(__VA_ARGS__) differently. Instead of expanding each VA_ARG in to its own parameter for the REGISTER_ID macro call, it calls REGISTER_ID with all the VA_ARGS as the first parameter. The RegisterID enum was expanding out to > typedef enum : int8_t { > eax, X86Registers::eax, "rax", 0, 0, ecx, X86Registers::ecx, ... > InvalidGPRReg = -1, > } RegisterID; https://godbolt.org/z/4FzLe4 Adding something like this in X86Registers.h and X86_64Registers.h got wincairo building locally > #if COMPILER(MSVC) > #define EXPAND_ARGS(x) x > #define macro(m, ...) EXPAND_ARGS(m(__VA_ARGS__)) > #else > #define macro(m, ...) m(__VA_ARGS__) > #endif I'm not too sure why apple win didn't also get hit by this. I created a bug to look at switching to MSVC's experimental preprocessor to try and avoid these in the future https://bugs.webkit.org/show_bug.cgi?id=198778.
Paulo Matos
Comment 22 2019-06-12 02:10:43 PDT
(In reply to Caio Lima from comment #18) > Comment on attachment 371733 [details] > Revised Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371733&action=review > > I didn't read everything, but I've left some comments. > Thanks for the comments. > > Source/JavaScriptCore/assembler/ARM64Assembler.h:169 > > +// in ARM64Registers.h on the macro definition of REGNS > > nit: There is some extra space here. > ok > > Source/JavaScriptCore/assembler/ARM64Assembler.h:173 > > +#define REGISTER_ID(id, nsid, name, r, cs) id, > > nit: Since we are not using `r` and `cs`, I think we could remove them. > > > Source/JavaScriptCore/assembler/ARM64Assembler.h:185 > > +#define REGISTER_ID(id, nsid, name) id, > > ditto. > > > Source/JavaScriptCore/assembler/ARM64Assembler.h:194 > > +#define REGISTER_ID(id, nsid, name, r, cs) id, > > Ditto. > > > Source/JavaScriptCore/assembler/ARM64Assembler.h:229 > > +#define REGISTER_NAME(id, nsid, name, r, cs) name, > > Ditto. > > > Source/JavaScriptCore/assembler/ARM64Assembler.h:251 > > +#define REGISTER_NAME(id, nsid, name, r, cs) name, > > Ditto. > I cannot remove the unused arguments. They are used by some architecture at some point. To make this truly generic, you need to put the arguments, otherwise RegisterSet.cpp won't compile. It uses these arguments to determine the calleesaved and reserved registers. > > Source/JavaScriptCore/assembler/ARM64Registers.h:29 > > +#define REGNS ARM64Registers > > One suggestion is to use `REGISTER_NAMESPACE` instead of `REGNS`. > I am happy with verbose! :) I actually had done that changed elsewhere when privately shared your initial comments but forgot to change it here. > > Source/JavaScriptCore/assembler/ARM64Registers.h:36 > > +#define macro(m, ...) m(__VA_ARGS__) > > Over JSC codebase we usually have `FOR_EACH...(macro)` style. Using `macro` > seems a little bit confusing to me. Can we rename it to `macro_forward` or > something else? ok
Paulo Matos
Comment 23 2019-06-12 02:20:09 PDT
(In reply to Don Olmstead from comment #20) > (In reply to Paulo Matos from comment #17) > > What are the architectural differences between win and wincairo which could > > make win pass and wincairo fail? I am unable to reproduce it on a local > > windows machine. Therefore I am marking the patch for review. Style problems > > are due to false positives reported in: > > https://bugs.webkit.org/show_bug.cgi?id=198605 > > It appears we're hitting differences between the preprocessor with MSVC and > Clang/GCC. Chris is working out a fix and it looks like technically MSVC has > an experimental preprocessor, > https://devblogs.microsoft.com/cppblog/msvc-preprocessor-progress-towards- > conformance/ , without this issue. > > Anyways if you can wait to see if Chris can get a version without the issue > that would be appreciated. You're right! Thanks for the reference. This exact issue is discussed in the article you posted under the header: Behavior 4 [comma elision in variadic macros] Like Chris said, why this didn't fail for win EWS is a mystery to me but thanks for looking into it.
Paulo Matos
Comment 24 2019-06-12 02:20:51 PDT
(In reply to Christopher Reid from comment #21) > Comment on attachment 371733 [details] > Revised Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371733&action=review > > > Source/JavaScriptCore/assembler/X86_64Registers.h:33 > > +#define macro(m, ...) m(__VA_ARGS__) > > The wincairo build is failing because MSVC expands the m(__VA_ARGS__) > differently. > Instead of expanding each VA_ARG in to its own parameter for the REGISTER_ID > macro call, it calls REGISTER_ID with all the VA_ARGS as the first parameter. > The RegisterID enum was expanding out to > > typedef enum : int8_t { > > eax, X86Registers::eax, "rax", 0, 0, ecx, X86Registers::ecx, ... > > InvalidGPRReg = -1, > > } RegisterID; > https://godbolt.org/z/4FzLe4 > > Adding something like this in X86Registers.h and X86_64Registers.h got > wincairo building locally > > #if COMPILER(MSVC) > > #define EXPAND_ARGS(x) x > > #define macro(m, ...) EXPAND_ARGS(m(__VA_ARGS__)) > > #else > > #define macro(m, ...) m(__VA_ARGS__) > > #endif > > I'm not too sure why apple win didn't also get hit by this. > I created a bug to look at switching to MSVC's experimental preprocessor to > try and avoid these in the future > https://bugs.webkit.org/show_bug.cgi?id=198778. Thanks Chris. I will use your workaround for now for MSVC and keep an eye on your bug. If we adopt the new preprocessor then we can come back and simplify the code.
Paulo Matos
Comment 25 2019-06-12 02:21:42 PDT
(In reply to Caio Lima from comment #19) > Comment on attachment 371733 [details] > Revised Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371733&action=review > > LGTM. I left more comments and we also need to figure out why wincario is > not building. > > >> Source/JavaScriptCore/assembler/ARM64Assembler.h:173 > >> +#define REGISTER_ID(id, nsid, name, r, cs) id, > > > > nit: Since we are not using `r` and `cs`, I think we could remove them. > > Never mind this comment. We need to have those. > > > Source/JavaScriptCore/assembler/MIPSAssembler.h:108 > > +#undef REGISTER_NAME > > Nice! > > > Source/JavaScriptCore/assembler/MIPSRegisters.h:108 > > + macro(m, nn(fir), 0) \ > > I think it would be very good have an commentary documenting each param > here, so we don't need to go to files where this `FOR_EACH` is used to > figure out what each value means. > > > Source/JavaScriptCore/jit/RegisterSet.cpp:51 > > +#define SET_IF_RESERVED(id, nsid, name, r, cs) if (r) result.set(nsid); > > I think we should rename `r` to `isReserved`. > > > Source/JavaScriptCore/jit/RegisterSet.cpp:113 > > +#define SET_IF_CALLEESAVED(id, nsid, name, r, cs) if (cs) result.set(nsid); > > I think it would be better rename `cs` to `isCalleeSaved`. Should have read this comments before commenting to your initial review set. In any case, thanks for the comments. I am preparing a new patch.
Paulo Matos
Comment 26 2019-06-12 03:23:52 PDT
Created attachment 371940 [details] Post-1 Review Patch
EWS Watchlist
Comment 27 2019-06-12 03:26:42 PDT
Attachment 371940 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:52: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:61: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:63: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:71: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:74: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:76: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:87: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:93: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:102: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:104: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:112: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:115: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:117: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:128: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:147: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:156: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:165: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 17 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Guillaume Emont
Comment 28 2019-06-12 03:32:26 PDT
Comment on attachment 371940 [details] Post-1 Review Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371940&action=review > Source/JavaScriptCore/assembler/ARM64Registers.h:38 > +#if COMPILER(MSVC) I suggest you put a comment here with a reference to https://bugs.webkit.org/show_bug.cgi?id=198778 . This way it would be obvious this can be removed once that issue is fixed.
Paulo Matos
Comment 29 2019-06-12 05:12:15 PDT
Created attachment 371943 [details] Post-1 Review Patch
EWS Watchlist
Comment 30 2019-06-12 05:15:03 PDT
Attachment 371943 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:52: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:61: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:63: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:71: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:74: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:76: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:87: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:93: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:102: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:104: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:112: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:115: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:117: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:128: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:147: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:156: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:165: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 17 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Paulo Matos
Comment 31 2019-06-12 05:17:48 PDT
Created attachment 371945 [details] Post-1 Review Patch
EWS Watchlist
Comment 32 2019-06-12 05:21:06 PDT
Attachment 371945 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:54: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:63: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:65: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:73: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:76: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:78: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:89: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:95: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:104: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:106: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:114: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:117: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:119: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:130: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:149: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:158: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:167: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 17 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Keith Miller
Comment 33 2019-06-12 06:28:13 PDT
Comment on attachment 371945 [details] Post-1 Review Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371945&action=review > Source/JavaScriptCore/assembler/ARM64Registers.h:31 > +#define REGISTER_NAMESPACE ARM64Registers I think this can just be a using RegisterNames = ARM64Registers; Then any user can just assume all registers are under the generic RegisterNames namespace. > Source/JavaScriptCore/assembler/ARM64Registers.h:55 > + macro_forward(macro, n(x0), 0, 0) \ Why do we need to have the macro_forward? Can't the consumer macro do any of the things n() does? Seems like that would be cleaner.
Paulo Matos
Comment 34 2019-06-12 07:33:02 PDT
(In reply to Keith Miller from comment #33) > Comment on attachment 371945 [details] > Post-1 Review Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371945&action=review > > > Source/JavaScriptCore/assembler/ARM64Registers.h:31 > > +#define REGISTER_NAMESPACE ARM64Registers > > I think this can just be a using RegisterNames = ARM64Registers; > > Then any user can just assume all registers are under the generic > RegisterNames namespace. > Not sure I understand the suggestion here. You mean, removing the platform dependent namespace name and making the namespace platform independent in both assembler/<arch>Assembler.h and elsewhere? > > Source/JavaScriptCore/assembler/ARM64Registers.h:55 > > + macro_forward(macro, n(x0), 0, 0) \ > > Why do we need to have the macro_forward? Can't the consumer macro do any of > the things n() does? Seems like that would be cleaner. Yes, we can expand n() manually and avoid it but n() serves as a helper so that we don't need to write: macro(x0, ARM64Registers::x0, "x0", 0, 0) Once we use macro(n(x0), 0, 0) then we need to use the macro_forward as a trick to force the preprocessor to split the macro arguments. Further to that, if we are on MSVC like Chris suggested we also need the EXPAND_ARGS macro helper. If people find it better to avoid the helpers and just call the macro with all the arguments, thereby avoiding the helper issues, I can do the change.
Paulo Matos
Comment 35 2019-06-12 07:34:19 PDT
There's some strange issue going on with the gtk-wk2 ews: ICECC[324] 06:12:39: flush_writebuf() failed Resource temporarily unavailable ICECC[324] 06:12:41: write of source chunk to host 192.168.10.42 ICECC[324] 06:12:41: failed Resource temporarily unavailable ICECC[324] 06:12:41: got exception Error 15 - write to host failed (192.168.10.42)
Paulo Matos
Comment 36 2019-06-12 07:54:03 PDT
Created attachment 371956 [details] Post-1 Review Patch
EWS Watchlist
Comment 37 2019-06-12 07:56:29 PDT
Attachment 371956 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:47: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:56: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:58: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:66: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:69: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:71: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:82: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:88: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:97: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:99: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:107: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:110: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:112: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:123: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:142: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:151: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:160: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 17 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.
Saam Barati
Comment 38 2019-06-12 12:03:39 PDT
Comment on attachment 371956 [details] Post-1 Review Patch Can you write a changelog with what this patch does and why?
Saam Barati
Comment 39 2019-06-12 12:05:09 PDT
Comment on attachment 371956 [details] Post-1 Review Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371956&action=review > Source/JavaScriptCore/jit/RegisterSet.cpp:118 > +#define SET_IF_CALLEESAVED(id, nsid, name, isReserved, isCalleeSaved) \ > + if (isCalleeSaved) \ > + result.set(nsid); > + FOR_EACH_GP_REGISTER(SET_IF_CALLEESAVED) > + FOR_EACH_FP_REGISTER(SET_IF_CALLEESAVED) > +#undef SET_IF_CALLEESAVED I like this! But a changelog will help us in evaluating the change. You can get the changelog template by running "Tools/Scripts/prepare-ChangeLog -b <bug-id>"
Paulo Matos
Comment 40 2019-06-13 00:53:29 PDT
Created attachment 372029 [details] Post-2 Review Patch
Paulo Matos
Comment 41 2019-06-13 00:54:01 PDT
(In reply to Saam Barati from comment #38) > Comment on attachment 371956 [details] > Post-1 Review Patch > > Can you write a changelog with what this patch does and why? Included in Post-2 Review Patch. Thanks.
EWS Watchlist
Comment 42 2019-06-13 00:56:36 PDT
Attachment 372029 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:47: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:56: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:58: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:66: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:69: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:71: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:82: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:88: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:97: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:99: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:107: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:110: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:112: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:123: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:142: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:151: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:160: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 17 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Keith Miller
Comment 43 2019-06-13 01:47:17 PDT
Comment on attachment 371945 [details] Post-1 Review Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371945&action=review >>> Source/JavaScriptCore/assembler/ARM64Registers.h:31 >>> +#define REGISTER_NAMESPACE ARM64Registers >> >> I think this can just be a using RegisterNames = ARM64Registers; >> >> Then any user can just assume all registers are under the generic RegisterNames namespace. > > Not sure I understand the suggestion here. > You mean, removing the platform dependent namespace name and making the namespace platform independent in both assembler/<arch>Assembler.h and elsewhere? I was thinking more that we could have both the platform specific name and an independent name. >>> Source/JavaScriptCore/assembler/ARM64Registers.h:55 >>> + macro_forward(macro, n(x0), 0, 0) \ >> >> Why do we need to have the macro_forward? Can't the consumer macro do any of the things n() does? Seems like that would be cleaner. > > Yes, we can expand n() manually and avoid it but n() serves as a helper so that we don't need to write: > macro(x0, ARM64Registers::x0, "x0", 0, 0) > > Once we use > macro(n(x0), 0, 0) > > then we need to use the macro_forward as a trick to force the preprocessor to split the macro arguments. Further to that, if we are on MSVC like Chris suggested we also need the EXPAND_ARGS macro helper. > > If people find it better to avoid the helpers and just call the macro with all the arguments, thereby avoiding the helper issues, I can do the change. I meant more that the consumer macro could just take x0 and do any part of the n() macro they want.
Paulo Matos
Comment 44 2019-06-13 03:43:21 PDT
(In reply to Keith Miller from comment #43) > Comment on attachment 371945 [details] > Post-1 Review Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=371945&action=review > > >>> Source/JavaScriptCore/assembler/ARM64Registers.h:31 > >>> +#define REGISTER_NAMESPACE ARM64Registers > >> > >> I think this can just be a using RegisterNames = ARM64Registers; > >> > >> Then any user can just assume all registers are under the generic RegisterNames namespace. > > > > Not sure I understand the suggestion here. > > You mean, removing the platform dependent namespace name and making the namespace platform independent in both assembler/<arch>Assembler.h and elsewhere? > > I was thinking more that we could have both the platform specific name and > an independent name. > You mean, using RegisterNames everywhere and defining RegisterNames to be platform specific in the platform specific file? That sounds good. Will fix. > I meant more that the consumer macro could just take x0 and do any part of > the n() macro they want. That is a great suggestion. It will, hopefully, simplify a bunch of things. Will get to it!
Paulo Matos
Comment 45 2019-06-13 03:43:58 PDT
For some reason the win ews bot failed, although there was no change to the code (last patch only added the ChangeLog).
Paulo Matos
Comment 46 2019-06-13 04:32:03 PDT
(In reply to Paulo Matos from comment #44) > (In reply to Keith Miller from comment #43) > > I meant more that the consumer macro could just take x0 and do any part of > > the n() macro they want. > > That is a great suggestion. It will, hopefully, simplify a bunch of things. > Will get to it! I was thinking a bit more about this and the issue is that sometimes the registername is the same as the enumerator identifier but that is not always the case. So while we can leave it to the consumer macro to create the register namespace identifier, as in ARM64Registers::x0, we still need to have the name as an argument at which point you need to write it like: macro(x0, "x0", 0, 0) \ macro(x1, "x1", 0, 0) \ macro(x2, "x2", 0, 0) \ ... But then if you want to simplify this, you introduce a helper macro n to do: #define n(id) id, #id and write it as: macro(n(x0), 0, 0) \ ... and as soon as this happens you need all the macro forwarding things. At this point, I think the pain of writing the name even when it's the same as the identifier is less than having all the macro forwarding stuff which hurts legibility. So, in my next proposal I will simplify the macro stuff by removing the macro forwarding work and the n/nn helpers and write down the name explicitly even when that matches the identifier.
Paulo Matos
Comment 47 2019-06-13 07:10:44 PDT
Created attachment 372049 [details] Post-3 Review Patch Major change in patch direction, where macrology is simplified by removing all n/nn() macro helpers.
EWS Watchlist
Comment 48 2019-06-13 07:12:44 PDT
Attachment 372049 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:87: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:88: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:89: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:90: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:91: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:92: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/RegisterSet.cpp:52: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:44: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:53: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:55: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:63: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:66: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:68: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:79: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:85: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:94: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:96: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:104: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:107: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:109: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:120: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:139: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:148: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:157: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 24 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Paulo Matos
Comment 49 2019-06-13 07:19:25 PDT
Created attachment 372051 [details] Post-3 Review Patch
EWS Watchlist
Comment 50 2019-06-13 07:21:00 PDT
Attachment 372051 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:87: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:88: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:89: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:90: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:91: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:92: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/RegisterSet.cpp:52: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:44: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:53: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:55: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:63: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:66: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:68: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:79: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:85: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:94: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:96: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:104: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:107: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:109: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:120: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:139: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:148: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:157: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 24 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Paulo Matos
Comment 51 2019-06-13 09:08:52 PDT
Both win and gtk-wk2 are failing but these seem to be unrelated to the patch.
Paulo Matos
Comment 52 2019-06-24 04:17:07 PDT
ping! any further comments on this?
Keith Miller
Comment 53 2019-06-24 10:11:51 PDT
Comment on attachment 372051 [details] Post-3 Review Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372051&action=review lgtm other than updating that comment. > Source/JavaScriptCore/assembler/RegisterInfo.h:45 > + * = 1. id: is an identifier used to specify the register (for example, as an enumerator > + * in an enum); > + * = 2. nsid: is an identifier specifying the full namespace identifier literal for id; > + * = 3. name: is a string constant specifying the name of the identifier; > + * = 4. isReserved: a boolean (usually 0/1) specifying if this is a reserved register; > + * = 5. isCalleeSaved: a boolean (usually 0/1) specifying if this is a callee saved register; I think you need to update these comments.
Paulo Matos
Comment 54 2019-06-26 03:56:40 PDT
Created attachment 372912 [details] Post-4 Review Patch Thanks Keith, just posted a new patch with the fixed comment.
EWS Watchlist
Comment 55 2019-06-26 03:59:27 PDT
Attachment 372912 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:87: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:88: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:89: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:90: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:91: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:92: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/RegisterSet.cpp:52: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:44: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:53: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:55: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:63: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:66: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:68: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:79: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:85: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:94: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:96: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:104: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:107: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:109: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:120: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:139: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:148: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:157: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 24 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Keith Miller
Comment 56 2019-06-26 08:51:22 PDT
Comment on attachment 372912 [details] Post-4 Review Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372912&action=review Think I found one more typo > Source/JavaScriptCore/assembler/RegisterInfo.h:39 > + * spread accross the five macro arguments. Typo: five => four. Unless we are counting from zero :P
Paulo Matos
Comment 57 2019-06-27 00:54:07 PDT
Created attachment 373008 [details] Post-5 Review Patch Thanks for the review @Keith. It is now fixed in post-5 patch.
EWS Watchlist
Comment 58 2019-06-27 00:56:41 PDT
Attachment 373008 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:87: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:88: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:89: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:90: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:91: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/assembler/X86_64Registers.h:92: Missing space after , [whitespace/comma] [3] ERROR: Source/JavaScriptCore/jit/RegisterSet.cpp:52: More than one command on the same line in if [whitespace/parens] [4] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:44: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:53: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:55: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:63: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:66: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:68: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:79: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:85: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:94: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:96: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:104: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:107: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:109: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:120: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:139: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:148: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:157: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 24 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Paulo Matos
Comment 59 2019-07-01 01:40:33 PDT
ping! Can someone approve this one or provide further feedback?
Keith Miller
Comment 60 2019-07-01 10:00:46 PDT
Comment on attachment 373008 [details] Post-5 Review Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373008&action=review Sorry about the delay. I thought this had already been review and committed for some reason... :/ I have a couple more style changes for you but I think it's almost there! Thanks again for the patch. > Source/JavaScriptCore/assembler/RegisterInfo.h:59 > +# include "X86Registers.h" > +#elif CPU(X86_64) > +# include "X86_64Registers.h" > +#elif CPU(MIPS) > +# include "MIPSRegisters.h" > +#elif CPU(ARM_THUMB2) > +# include "ARMv7Registers.h" > +#elif CPU(ARM64) > +# include "ARM64Registers.h" These includes should not have spaces between the # and the "include" > Source/JavaScriptCore/assembler/X86_64Registers.h:92 > + macro(xmm10,"xmm10", 0, 0) \ > + macro(xmm11,"xmm11", 0, 0) \ > + macro(xmm12,"xmm12", 0, 0) \ > + macro(xmm13,"xmm13", 0, 0) \ > + macro(xmm14,"xmm14", 0, 0) \ > + macro(xmm15,"xmm15", 0, 0) Can you add a space after these register identifiers? That would match webkit style. > Source/JavaScriptCore/jit/RegisterSet.cpp:52 > + if (isReserved) result.set(RegisterNames::id); I think the body of the if should be on its own line and be indented. > Source/JavaScriptCore/jit/RegisterSet.cpp:116 > + result.set(RegisterNames::id); Can you fix the indentation here?
Paulo Matos
Comment 61 2019-07-02 05:20:41 PDT
Thank you for your time reviewing this. (In reply to Keith Miller from comment #60) > Comment on attachment 373008 [details] > Post-5 Review Patch > > Source/JavaScriptCore/assembler/X86_64Registers.h:92 > > + macro(xmm10,"xmm10", 0, 0) \ > > + macro(xmm11,"xmm11", 0, 0) \ > > + macro(xmm12,"xmm12", 0, 0) \ > > + macro(xmm13,"xmm13", 0, 0) \ > > + macro(xmm14,"xmm14", 0, 0) \ > > + macro(xmm15,"xmm15", 0, 0) > > Can you add a space after these register identifiers? That would match > webkit style. > Thank you for your time reviewing this. Shall I align all the others columns as well then for ease of reading the table like so: #define FOR_EACH_FP_REGISTER(macro) \ macro(xmm0, "xmm0", 0, 0) \ macro(xmm1, "xmm1", 0, 0) \ macro(xmm2, "xmm2", 0, 0) \ macro(xmm3, "xmm3", 0, 0) \ macro(xmm4, "xmm4", 0, 0) \ macro(xmm5, "xmm5", 0, 0) \ macro(xmm6, "xmm6", 0, 0) \ macro(xmm7, "xmm7", 0, 0) \ macro(xmm8, "xmm8", 0, 0) \ macro(xmm9, "xmm9", 0, 0) \ macro(xmm10, "xmm10", 0, 0) \ macro(xmm11, "xmm11", 0, 0) \ macro(xmm12, "xmm12", 0, 0) \ macro(xmm13, "xmm13", 0, 0) \ macro(xmm14, "xmm14", 0, 0) \ macro(xmm15, "xmm15", 0, 0) That would mean two spaces between 'xmm0,' and '"xmm0"' but I think it improves clarity. I have done this elsewhere.
Keith Miller
Comment 62 2019-07-02 11:30:45 PDT
(In reply to Paulo Matos from comment #61) > > Thank you for your time reviewing this. Shall I align all the others columns > as well then for ease of reading the table like so: > > That would mean two spaces between 'xmm0,' and '"xmm0"' but I think it > improves clarity. I have done this elsewhere. Yeah, I think having two spaces makes sense.
Paulo Matos
Comment 63 2019-07-03 00:16:27 PDT
Created attachment 373385 [details] Post-6 Review Patch @Keith Thanks for your time and comments. Hopefully this is now good to go.
EWS Watchlist
Comment 64 2019-07-03 00:18:12 PDT
Attachment 373385 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:44: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:53: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:55: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:63: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:66: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:68: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:79: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:85: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:94: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:96: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:104: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:107: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:109: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:120: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:139: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:148: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/assembler/ARM64Registers.h:157: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 17 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Keith Miller
Comment 65 2019-07-03 12:21:54 PDT
Comment on attachment 373385 [details] Post-6 Review Patch r=me
WebKit Commit Bot
Comment 66 2019-07-03 12:52:48 PDT
Comment on attachment 373385 [details] Post-6 Review Patch Clearing flags on attachment: 373385 Committed r247097: <https://trac.webkit.org/changeset/247097>
WebKit Commit Bot
Comment 67 2019-07-03 12:52:51 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 68 2019-07-03 12:53:28 PDT
<rdar://problem/52605121>
Note You need to log in before you can comment on or make changes to this bug.