Bug 3576 - roll in support for "const" keyword from KDE tree
Summary: roll in support for "const" keyword from KDE tree
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 412
Hardware: Mac OS X 10.4
: P2 Enhancement
Assignee: Maciej Stachowiak
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-06-16 13:56 PDT by Darin Adler
Modified: 2005-07-20 20:19 PDT (History)
0 users

See Also:


Attachments
Patch for const support, cleaner fix than KDE version IMO (28.25 KB, patch)
2005-06-20 18:58 PDT, Maciej Stachowiak
darin: review-
Details | Formatted Diff | Diff
Revised patch: fixes formatting, adds new test case, fixes another bug (31.30 KB, patch)
2005-06-20 20:14 PDT, Maciej Stachowiak
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2005-06-16 13:56:16 PDT
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.
Comment 1 Maciej Stachowiak 2005-06-20 18:58:34 PDT
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.
Comment 2 Darin Adler 2005-06-20 19:08:46 PDT
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?
Comment 3 Maciej Stachowiak 2005-06-20 19:16:48 PDT
"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.
Comment 4 Maciej Stachowiak 2005-06-20 20:14:49 PDT
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.
Comment 5 Dave Hyatt 2005-06-21 01:30:39 PDT
Comment on attachment 2515 [details]
Revised patch: fixes formatting, adds new test case, fixes another bug

r=me
Comment 6 Joost de Valk (AlthA) 2005-07-03 08:10:59 PDT
Darin, you know what to do :) is this verifiable? :)
Comment 7 Maciej Stachowiak 2005-07-03 15:47:59 PDT
A test case was committed with the patch, you can try that to verify.