Bug 192271 - Fix the bytecode code generator scripts to pretty print BytecodeStructs.h and BytecodeIndices.h.
Summary: Fix the bytecode code generator scripts to pretty print BytecodeStructs.h and...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-30 17:30 PST by Mark Lam
Modified: 2018-12-03 09:59 PST (History)
8 users (show)

See Also:


Attachments
proposed patch. (17.02 KB, patch)
2018-11-30 17:34 PST, Mark Lam
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2018-11-30 17:30:18 PST
This makes the generated code style compliant and human readable.
Comment 1 Mark Lam 2018-11-30 17:34:44 PST
Created attachment 356271 [details]
proposed patch.
Comment 2 Keith Miller 2018-12-01 12:53:54 PST
Comment on attachment 356271 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=356271&action=review

r=me

> Source/JavaScriptCore/generator/Opcode.rb:208
> +#{@metadata.struct(self)}#{@metadata.accessor}

Shouldn’t there be a new line between these?

> Source/JavaScriptCore/generator/Opcode.rb:214
> +#{print_members("    ", &:field)}#{@metadata.field("    ")}

Ditto.
Comment 3 Saam Barati 2018-12-01 13:34:48 PST
Comment on attachment 356271 [details]
proposed patch.

Can you do this in a way that doesn't make the ruby code worse to read? Why not just do some post processing on the string?
Comment 4 Saam Barati 2018-12-01 13:35:03 PST
(In reply to Saam Barati from comment #3)
> Comment on attachment 356271 [details]
> proposed patch.
> 
> Can you do this in a way that doesn't make the ruby code worse to read? Why
> not just do some post processing on the string?

I think this would also lead to fewer LOC changed
Comment 5 Mark Lam 2018-12-03 09:47:50 PST
Comment on attachment 356271 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=356271&action=review

Thanks for the review.

>> Source/JavaScriptCore/generator/Opcode.rb:208
>> +#{@metadata.struct(self)}#{@metadata.accessor}
> 
> Shouldn’t there be a new line between these?

This is needed because the #{@metadata.struct(self)} maybe an empty string, and we don't want the extra '\n'.

>> Source/JavaScriptCore/generator/Opcode.rb:214
>> +#{print_members("    ", &:field)}#{@metadata.field("    ")}
> 
> Ditto.

Same here.  #{print_members(" ", &:field)} may be an empty string and we don't want the extra '\n'.
Comment 6 Mark Lam 2018-12-03 09:53:43 PST
(In reply to Saam Barati from comment #4)
> (In reply to Saam Barati from comment #3)
> > Comment on attachment 356271 [details]
> > proposed patch.
> > 
> > Can you do this in a way that doesn't make the ruby code worse to read? Why
> > not just do some post processing on the string?
> 
> I think this would also lead to fewer LOC changed

Perhaps, but my initial thinking is that post-processing would require a parser of sorts that recognizes when to indent and unindent.  I don't think it's trivial in the general case, but perhaps we could implement mini post-processors that might be doable.

I'm going to leave this patch as is for now, and move on.  I think we'll be working with the ruby scripts less than we will with the generated C++ file.  I'm going to land it so that we get the benefit of a properly formatted C++ file, and revisit the formatting of the ruby script at a later date if we find it to be a detriment to our productivity.
Comment 7 Mark Lam 2018-12-03 09:58:21 PST
Landed in r238804: <http://trac.webkit.org/r238804>.
Comment 8 Radar WebKit Bug Importer 2018-12-03 09:59:46 PST
<rdar://problem/46421957>