Bug 192271 – Fix the bytecode code generator scripts to pretty print BytecodeStructs.h and BytecodeIndices.h.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

This makes the generated code style compliant and human readable.

[Mark Lam]

Created attachment 356271 [details]
proposed patch.

[Keith Miller]

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.

[Saam Barati]

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?

[Saam Barati]

(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

[Mark Lam]

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

[Mark Lam]

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

[Mark Lam]

Landed in r238804: <http://trac.webkit.org/r238804>.

[Radar WebKit Bug Importer]

<rdar://problem/46421957>