Bug 193467 – Refactor new bytecode structs so that the fields are prefixed with "m_".

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

[Mark Lam]

This makes it easier to do a manual audit of type correctness of the LLInt instructions used to access these fields.  Without this change, it would be difficult (and error prone) to distinguish the difference between field names and macro variables.

[Mark Lam]

Created attachment 359210 [details]
proposed patch.

[Saam Barati]

Comment on attachment 359210 [details]
proposed patch.

r=me but we should probably wait for Tadeu to say it's ok too.

[EWS Watchlist]

Comment on attachment 359210 [details]
proposed patch.

Attachment 359210 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10764813

New failing tests:
stress/arith-abs-to-arith-negate-range-optimizaton.js.no-llint
apiTests

[Mark Lam]

(In reply to Build Bot from comment #3)
> Comment on attachment 359210 [details]
> proposed patch.
> 
> Attachment 359210 [details] did not pass jsc-ews (mac):
> Output: https://webkit-queues.webkit.org/results/10764813
> 
> New failing tests:
> stress/arith-abs-to-arith-negate-range-optimizaton.js.no-llint
> apiTests

Found the issue: I missed one place: the SFINAE condition for JIT::emitValueProfilingSiteIfProfiledOpcode which depends on Metadata::m_profile.  Will upload patch for landing with this fix shortly.

[Mark Lam]

Created attachment 359230 [details]
patch for landing.

[Mark Lam]

Created attachment 359243 [details]
patch for landing.

Rebased to ToT.

[EWS Watchlist]

Attachment 359243 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1307:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tadeu Zagallo]

Comment on attachment 359243 [details]
patch for landing.

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

Looks good to me, I think the rationale makes sense. Just a couple nits in the ruby code.

> Source/JavaScriptCore/generator/Argument.rb:38
> +        "#{@type.to_s} m_#{@name};"

Just a nit, but this should probably use `field_name` here. You might also want to remove the `attr_reader :name` to avoid accidentally referring to the unprefixed name.

> Source/JavaScriptCore/generator/Opcode.rb:83
> +    def untyped_params

This seems to be unused

[Mark Lam]

Thanks.

(In reply to Tadeu Zagallo from comment #8)
> Comment on attachment 359243 [details]
> patch for landing.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359243&action=review
> 
> Looks good to me, I think the rationale makes sense. Just a couple nits in
> the ruby code.
> 
> > Source/JavaScriptCore/generator/Argument.rb:38
> > +        "#{@type.to_s} m_#{@name};"

Fixed.

> Just a nit, but this should probably use `field_name` here. You might also
> want to remove the `attr_reader :name` to avoid accidentally referring to
> the unprefixed name.

Hmmm, I'm not too knowledgeable about ruby but we still need a name property, and it should be write once i.e. read only after the Argument is constructed.  So, I think attr_reader :name is appropriate.

> > Source/JavaScriptCore/generator/Opcode.rb:83
> > +    def untyped_params
> 
> This seems to be unused

Removed.

[Mark Lam]

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

[Radar WebKit Bug Importer]

<rdar://problem/47321555>