Make the $vm debugging tools available to builtins as @$vm.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

... because we also need some debugging tools for builtin development.  The $vm object will be made available to builtins as @dbg, which gives us, amongst many goodies, @dbg.print() (which prints the toString() values of its args) and @dbg.printValue() (which dataLogs its arg as a JSValue).  @dbg will only be available if we run with JSC_enableDollarVM=true.

Note: it would have been nice to use @$vm (instead of @dbg) but our CommonIdentifiers infrastructure does not current support $ in the name of identifiers (and it's not worth the hassle to add it).

[Geoffrey Garen]

Let's not use two names for the same object.

[Mark Lam]

(In reply to comment #1)
> Let's not use two names for the same object.

My options for keeping the name the same (where same means builtins would use the same name prepended by @) are:

1. Change the JSC CommonIdentifier infrastructure to be able to handle the $ character in the identifiers.

2. Rename $vm to something that CommonIdentifier can handle, e.g. _dbg.  With that, the builtins can use @_dbg.

The reason for choosing the $vm name in the first place was so that it has a low likelihood of collision with any names that a JS library may use.  I also wanted a short name so that it's not too verbose to type.  Given these needs, I think _dbg will fit the bill.  A quick search of the web also shows no current popular use of _dbg in JS code.

I'll go with option 2.  Before proceeding with this patch, I'll make a separate patch to some renaming first:
1. $vm ==> _dbg
2. JSDollarVM ==> JSUnderscoreDbg.
3. JSDollarVMPrototype ==> JSUnderscoreDbgPrototype.

[Mark Lam]

(In reply to comment #2)
> My options for keeping the name the same (where same means builtins would
> use the same name prepended by @) are:
> 
> 1. Change the JSC CommonIdentifier infrastructure to be able to handle the $
> character in the identifiers.
> 
> 2. Rename $vm to something that CommonIdentifier can handle, e.g. _dbg. 
> With that, the builtins can use @_dbg.

Correction: I have option 3:

3. Special case $vm in BuiltinNames.h.

Option 3 is the simplest and least invasive to do.  Moving forward with it.

[Mark Lam]

Created attachment 275203 [details]
proposed patch.

[Saam Barati]

Comment on attachment 275203 [details]
proposed patch.

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

> Source/JavaScriptCore/builtins/BuiltinNames.h:72
> +    const JSC::Identifier& dollarVMPublicName() const { return m_dollarVMName; }

I don't think the public name is needed.

It's also possible to use "dollarVM" as the identifier and not end up with all this duplicate code outside the macro.

[Mark Lam]

(In reply to comment #5)
> Comment on attachment 275203 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275203&action=review
> 
> > Source/JavaScriptCore/builtins/BuiltinNames.h:72
> > +    const JSC::Identifier& dollarVMPublicName() const { return m_dollarVMName; }
> 
> I don't think the public name is needed.

The lexer lowers @$vm to $vm and maps it the public name to the private name using the public to private map.  The parser then checks for the private name in the private to public map.  Hence, we do need both public and private names.  

> It's also possible to use "dollarVM" as the identifier and not end up with
> all this duplicate code outside the macro.

I think @$vm is a better choice because it's good to keep the way we reference the object the same for consistency.

I'll go ahead and just land this patch.

[WebKit Commit Bot]

Comment on attachment 275203 [details]
proposed patch.

Clearing flags on attachment: 275203

Committed r198855: <http://trac.webkit.org/changeset/198855>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.