Bug 198604 - Refactoring of architectural Register Information
Summary: Refactoring of architectural Register Information
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-06 03:34 PDT by Paulo Matos
Modified: 2019-07-03 12:53 PDT (History)
12 users (show)

See Also:

Attachments
Patch (54.83 KB, patch)
2019-06-06 03:34 PDT, Paulo Matos 		ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Revised Patch (58.21 KB, patch)
2019-06-06 14:30 PDT, Paulo Matos 		no flags Details | Formatted Diff | Diff
Revised Patch (58.21 KB, patch)
2019-06-07 05:26 PDT, Paulo Matos 		no flags Details | Formatted Diff | Diff
Revised Patch (72.44 KB, patch)
2019-06-10 00:31 PDT, Paulo Matos 		no flags Details | Formatted Diff | Diff
Revised Patch (72.44 KB, patch)
2019-06-10 01:31 PDT, Paulo Matos 		no flags Details | Formatted Diff | Diff
Revised Patch (72.78 KB, patch)
2019-06-10 04:24 PDT, Paulo Matos 		no flags Details | Formatted Diff | Diff
Post-1 Review Patch (74.22 KB, patch)
2019-06-12 03:23 PDT, Paulo Matos 		no flags Details | Formatted Diff | Diff
Post-1 Review Patch (74.22 KB, patch)
2019-06-12 05:12 PDT, Paulo Matos 		no flags Details | Formatted Diff | Diff
Post-1 Review Patch (74.36 KB, patch)
2019-06-12 05:17 PDT, Paulo Matos 		no flags Details | Formatted Diff | Diff
Post-1 Review Patch (74.42 KB, patch)
2019-06-12 07:54 PDT, Paulo Matos 		saam: review-
Details | Formatted Diff | Diff
Post-2 Review Patch (77.49 KB, patch)
2019-06-13 00:53 PDT, Paulo Matos 		no flags Details | Formatted Diff | Diff
Post-3 Review Patch (75.65 KB, patch)
2019-06-13 07:10 PDT, Paulo Matos 		no flags Details | Formatted Diff | Diff
Post-3 Review Patch (75.65 KB, patch)
2019-06-13 07:19 PDT, Paulo Matos 		no flags Details | Formatted Diff | Diff
Post-4 Review Patch (75.51 KB, patch)
2019-06-26 03:56 PDT, Paulo Matos 		no flags Details | Formatted Diff | Diff
Post-5 Review Patch (75.51 KB, patch)
2019-06-27 00:54 PDT, Paulo Matos 		keith_miller: review-
Details | Formatted Diff | Diff
Post-6 Review Patch (75.58 KB, patch)
2019-07-03 00:16 PDT, Paulo Matos 		no flags Details | Formatted Diff | Diff
Show Obsolete (17) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Paulo Matos 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.
Comment 1 EWS Watchlist 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.
Comment 2 Paulo Matos 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
Comment 3 EWS Watchlist 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
Comment 4 EWS Watchlist 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
Comment 5 Paulo Matos 2019-06-06 14:00:12 PDT 
Created attachment 371525 [details]
Revised Patch
Comment 6 EWS Watchlist 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.
Comment 7 Paulo Matos 2019-06-06 14:30:18 PDT 
Created attachment 371526 [details]
Revised Patch
Comment 8 EWS Watchlist 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.
Comment 9 Paulo Matos 2019-06-07 05:26:57 PDT 
Created attachment 371583 [details]
Revised Patch
Comment 10 EWS Watchlist 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.
Comment 11 Paulo Matos 2019-06-10 00:31:11 PDT 
Created attachment 371727 [details]
Revised Patch
Comment 12 EWS Watchlist 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.
Comment 13 Paulo Matos 2019-06-10 01:31:02 PDT 
Created attachment 371729 [details]
Revised Patch
Comment 14 EWS Watchlist 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.
Comment 15 Paulo Matos 2019-06-10 04:24:56 PDT 
Created attachment 371733 [details]
Revised Patch
Comment 16 EWS Watchlist 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.
Comment 17 Paulo Matos 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
Comment 18 Caio Lima 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?
Comment 19 Caio Lima 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`.
Comment 20 Don Olmstead 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.
Comment 21 Christopher Reid 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.
Comment 22 Paulo Matos 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
Comment 23 Paulo Matos 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.
Comment 24 Paulo Matos 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.
Comment 25 Paulo Matos 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.
Comment 26 Paulo Matos 2019-06-12 03:23:52 PDT 
Created attachment 371940 [details]
Post-1 Review Patch
Comment 27 EWS Watchlist 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.
Comment 28 Guillaume Emont 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.
Comment 29 Paulo Matos 2019-06-12 05:12:15 PDT 
Created attachment 371943 [details]
Post-1 Review Patch
Comment 30 EWS Watchlist 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.
Comment 31 Paulo Matos 2019-06-12 05:17:48 PDT 
Created attachment 371945 [details]
Post-1 Review Patch
Comment 32 EWS Watchlist 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.
Comment 33 Keith Miller 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.
Comment 34 Paulo Matos 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.
Comment 35 Paulo Matos 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)
Comment 36 Paulo Matos 2019-06-12 07:54:03 PDT 
Created attachment 371956 [details]
Post-1 Review Patch
Comment 37 EWS Watchlist 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.
Comment 38 Saam Barati 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?
Comment 39 Saam Barati 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>"
Comment 40 Paulo Matos 2019-06-13 00:53:29 PDT 
Created attachment 372029 [details]
Post-2 Review Patch
Comment 41 Paulo Matos 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.
Comment 42 EWS Watchlist 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.
Comment 43 Keith Miller 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.
Comment 44 Paulo Matos 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!
Comment 45 Paulo Matos 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).
Comment 46 Paulo Matos 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.
Comment 47 Paulo Matos 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.
Comment 48 EWS Watchlist 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.
Comment 49 Paulo Matos 2019-06-13 07:19:25 PDT 
Created attachment 372051 [details]
Post-3 Review Patch
Comment 50 EWS Watchlist 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.
Comment 51 Paulo Matos 2019-06-13 09:08:52 PDT 
Both win and gtk-wk2 are failing but these seem to be unrelated to the patch.
Comment 52 Paulo Matos 2019-06-24 04:17:07 PDT 
ping! any further comments on this?
Comment 53 Keith Miller 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.
Comment 54 Paulo Matos 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.
Comment 55 EWS Watchlist 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.
Comment 56 Keith Miller 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
Comment 57 Paulo Matos 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.
Comment 58 EWS Watchlist 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.
Comment 59 Paulo Matos 2019-07-01 01:40:33 PDT 
ping! Can someone approve this one or provide further feedback?
Comment 60 Keith Miller 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?
Comment 61 Paulo Matos 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.
Comment 62 Keith Miller 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.
Comment 63 Paulo Matos 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.
Comment 64 EWS Watchlist 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.
Comment 65 Keith Miller 2019-07-03 12:21:54 PDT 
Comment on attachment 373385 [details]
Post-6 Review Patch

r=me
Comment 66 WebKit Commit Bot 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>
Comment 67 WebKit Commit Bot 2019-07-03 12:52:51 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 68 Radar WebKit Bug Importer 2019-07-03 12:53:28 PDT 
<rdar://problem/52605121>