Bug 3576 – roll in support for "const" keyword from KDE tree

• Status: VERIFIED
• Resolution: FIXED
• Component: JavaScriptCore

[Darin Adler]

Harri Porten added support for "const" to KDE's KJS.

Here's what he had to say about it:

I already told Maciej on #khtml that I had implemented support for the
non-standard "const" keyword some time ago. At first I simply made it a
synonym to "var" to avoid syntax errors. Later I added a real
implementation that makes the variable read-only.

To keep the necessary changes minimal I had introduced a global flag. As
expected that caused a bug reported last week. Fixed now.

 http://websvn.kde.org/trunk/kdelibs/kjs/keywords.table?rev=290511&view=rev
 http://websvn.kde.org/trunk/kdelibs/kjs/nodes.cpp?rev=297361&view=rev
 http://websvn.kde.org/trunk/KDE/kdelibs/kjs/nodes.cpp?rev=426263&view=rev

The diffs should be extractable with "svn diff". If necessary, I can
provide the patches. If you change anything while merging these changes
please send us the patches so that we can sync the implementation.

[Maciej Stachowiak]

Created attachment 2513 [details]
Patch for const support, cleaner fix than KDE version IMO

Patch attached. I changed things somewhat from the kjs version of the fix.

[Darin Adler]

Comment on attachment 2513 [details]
Patch for const support, cleaner fix than KDE version IMO

Looks, good. Here are my comments:

One hanging "-see also test case" seen in JavaScriptCore/ChangeLog. You
probably meant to reference the test case in WebCore.

VarDeclNode::processVarDecls seems to be different from before. It's setting
the Internal bit on the property it's setting, and also sometimes not setting
the DontDelete flag. Is that a correct change? If so, then maybe we should have
a test case that depends on it.

Strange formatting in the line with the "already declared ?" comment -- it adds
spaces in a format we don't usually use.

Missing space after "exec," in variable.put line.

Is there a reason for the change in formatting in the VarStatementNode
constructor?

There are tabs in the ChangeLog entries -- should be spaces.

Does this cause problems with use of the identifier "const" as, say, the name
of a variable, function, or property?

[Maciej Stachowiak]

"VarDeclNode::processVarDecls seems to be different from before." - this change was in the patch, but it 
also makes the behavior of processVarDecls match evaluate. It's certainly correct if it makes a difference 
but I'm not sure if it does.

"Does this cause problems with use of the identifier "const" as, say, the name of a variable, function, or 
property?" - that was already impossible since "const" was listed as reserved in the keyword table.

[Maciej Stachowiak]

Created attachment 2515 [details]
Revised patch: fixes formatting, adds new test case, fixes another bug

Attached a revised patch which fixes the formatting issues, adds a test that
covers the processVarDecl change, and fixes another bug (eval not calling
processVarDecls) that was needed to make the processVarDecls change testable
(otherwise there'd be no way to check if delete works in an eval block when
VarDeclNode::processVarDecls has been called but VarDeclNode::evaluate hasn't).


I checked that both the extra fixes match mozilla behavior.

[Dave Hyatt]

Comment on attachment 2515 [details]
Revised patch: fixes formatting, adds new test case, fixes another bug

r=me

[Joost de Valk (AlthA)]

Darin, you know what to do :) is this verifiable? :)

[Maciej Stachowiak]

A test case was committed with the patch, you can try that to verify.