Bug 28944 – Squirrelfish bytecode documentation is out of date.

• Component: WebKit Website
• Status: RESOLVED INVALID

[Robert Agoston]

Created attachment 38983 [details]
proposed patch

Squirrelfish bytecode documentation is out of date.

[Darin Adler]

Comment on attachment 38983 [details]
proposed patch

> +<h2><code>eq_null</code></h2>
> +<p><b>Format: </b><code>
> +eq_null dst(r) src(r)
> +</code></p>
> +<p>
> +
> +            Checks whether register src is null, as with the ECMAScript '!='
> +            operator, and puts the result as a boolean in register dst.

'==', not '!='

> +            Determines whether the type string for src according to
> +            the ECMAScript rules is "undefined", and puts the result
> +            in register dst.

Saying this in terms of "type string" for these opcodes seems like an oblique way to say that this checks if the type is undefined. Is there some reason you need to do that to be precise or accurate?

> +<h2><code>resolve_global</code></h2>
> +<p><b>Format: </b><code>
> +resolve_skip dst(r) globalObject(c) property(id) structure(sID) offset(n)

resolve_global, not resolve_skip

> +</code></p>
> +<p>
> +         
> +            Performs a dynamic property lookup for the given property, on the provided
> +            global object.  If structure matches the Structure of the global then perform
> +            a fast lookup using the case offset, otherwise fall back to a full resolve and
> +            cache the new structure and offset

Missing a period at the end. We use single spaces after periods in this project.

> +</p>
> +<h2><code>get_global_var</code></h2>
> +<p><b>Format: </b><code>
> +get_global_var dst(r) globalObject(c) index(n)
> +</code></p>
> +<p>
> +
> +            Gets the global var at global slot index and places it in register dst.

We should say "global variable", not "global var".

> +</p>
> +<h2><code>put_global_var</code></h2>
> +<p><b>Format: </b><code>
> +put_global_var globalObject(c) index(n) value(r)
> +</code></p>
> +<p>
> +         
> +            Puts value into global slot index.

In get_global_var you mention "global variable" but here you just say "put into". I think the wording should be consistent. You could just say "Gets the value from global slot index" or you could say "global variable at global slot index" here.

> +<h2><code>get_by_id_self</code></h2>
>  <p><b>Format: </b><code>
> +op_get_by_id_self dst(r) base(r) property(id) structure(sID) offset(n) nop(n) nop(n)

get_by_id_self, not op_get_by_id_self

No discussion of the structure and offset arguments. They should be documented.


> +            Cached property access: Attempts to get a cached property from the
> +            value base. If the cache misses, op_get_by_id_self reverts to
> +            op_get_by_id.

The uses of the op_ prefix here seem unnecessary. The terminology "reverts to" is a little confusing. It's strange that in other opcodes we explain each argument but here we don't.

Sorry, I don't have time to review all the rest of this.

[Robert Agoston]

(In reply to comment #1)
in fact, the http://webkit.org/specs/squirrelfish-bytecode.html is generated from JavaScriptCore/interpreter/Interpreter.cpp comments by JavaScriptCore/docs/make-bytecode-docs.pl script.

I just used this tool for my patch.

[Eric Seidel (no email)]

I don't know much about SF, but CC'd some folks who do.

[Eric Seidel (no email)]

Comment on attachment 38983 [details]
proposed patch

r- based on Darin's above comments.  This patch needs further revision.

[Jon Davis]

This is no longer valid.