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.

Some of the complains are false positives. Reported that issue separately: https://bugs.webkit.org/show_bug.cgi?id=198605

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

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

Created attachment 371525 [details] Revised Patch

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.

Created attachment 371526 [details] Revised Patch

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.

Created attachment 371583 [details] Revised Patch

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.

Created attachment 371727 [details] Revised Patch

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.

Created attachment 371729 [details] Revised Patch

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.

Created attachment 371733 [details] Revised Patch

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.

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

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

(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

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

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

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

Created attachment 371940 [details] Post-1 Review Patch

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

Created attachment 371943 [details] Post-1 Review Patch

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.

Created attachment 371945 [details] Post-1 Review Patch

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

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

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)

Created attachment 371956 [details] Post-1 Review Patch

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 on attachment 371956 [details] Post-1 Review Patch Can you write a changelog with what this patch does and why?

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>"

Created attachment 372029 [details] Post-2 Review Patch

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

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

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

For some reason the win ews bot failed, although there was no change to the code (last patch only added the ChangeLog).

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

Created attachment 372049 [details] Post-3 Review Patch Major change in patch direction, where macrology is simplified by removing all n/nn() macro helpers.

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.

Created attachment 372051 [details] Post-3 Review Patch

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.

Both win and gtk-wk2 are failing but these seem to be unrelated to the patch.

ping! any further comments on this?

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.

Created attachment 372912 [details] Post-4 Review Patch Thanks Keith, just posted a new patch with the fixed comment.

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

Created attachment 373008 [details] Post-5 Review Patch Thanks for the review @Keith. It is now fixed in post-5 patch.

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.

ping! Can someone approve this one or provide further feedback?

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?

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.

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

Created attachment 373385 [details] Post-6 Review Patch @Keith Thanks for your time and comments. Hopefully this is now good to go.

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 on attachment 373385 [details] Post-6 Review Patch r=me

Comment on attachment 373385 [details] Post-6 Review Patch Clearing flags on attachment: 373385 Committed r247097: <https://trac.webkit.org/changeset/247097>

All reviewed patches have been landed. Closing bug.

<rdar://problem/52605121>