Summary: | roll in support for "const" keyword from KDE tree | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||
Component: | JavaScriptCore | Assignee: | Maciej Stachowiak <mjs> | ||||||
Status: | VERIFIED FIXED | ||||||||
Severity: | Enhancement | ||||||||
Priority: | P2 | ||||||||
Version: | 412 | ||||||||
Hardware: | Mac | ||||||||
OS: | OS X 10.4 | ||||||||
Attachments: |
|
Description
Darin Adler
2005-06-16 13:56:16 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 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?
"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. 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 on attachment 2515 [details]
Revised patch: fixes formatting, adds new test case, fixes another bug
r=me
Darin, you know what to do :) is this verifiable? :) A test case was committed with the patch, you can try that to verify. |