We currently emit code for const, but we don't check whether something is const before assigning to it. While there are ways around this in trunk JavaScriptCore as well, we should at least try to match the same level of support. This affects the layout tests fast/js/const and fast/js/kde/const.
To pass the layout tests, we only need to do the case of locals and this: var object = { inWith1: "RIGHT", inWith2: ""} with (object) { const inWith1 = "WRONG"; const inWith2 = "RIGHT"; inWith2 = "WRONG"; } shouldBe("object.inWith1", "'RIGHT'"); shouldBe("inWith2", "'RIGHT'"); We also don't need to worry about something like this, because it's broken in trunk: const x = 1; var x = 2; const y = 1; const y = 2; I'm assigning this to myself.
Created attachment 20881 [details] Patch in progress Here is a patch that implements all of the necessary codegen for const. It passes almost all of the const tests, the only exception being things of the form: const x = 1; function f() { x = 2; } This is because the attributes are not respected at runtime in JSVariableObject. I'm not quite sure of the best way to store them.
Comment on attachment 20881 [details] Patch in progress Removing review flag. I am going to make it store the ReadOnly bit in the index in the SymbolTable.
Created attachment 20884 [details] Proposed patch Here's a patch that fixes the 2 const tests and doesn't regress on any other tests. It works, but I think it would be served with some better abstraction of the indices in the SymbolTable from the actual entries. I am going to do some performance testing now. If stealing a bit is too expensive then we may need to have two separate hash tables for non-const and const entries.
Strangely enough, run-sunspider --runs 40 says this patch is an approximately 0.5% progression.
Created attachment 20885 [details] Revised proposed patch I needed to make a small change in order to get the patch to compile in release mode.
Comment on attachment 20885 [details] Revised proposed patch Okay this looks basically fine, but id really like geoff to look at it.
Created attachment 20905 [details] Revised proposed patch Here is a new version that patches cleanly against ToT.
This patch looks good to me, with one caveat: I'd really like to see SymbolTable become a true class that has a HashMap data member, and does the bit masking for you automatically. Right now, SymbolTable is kind of error-prone, since all clients of get, set, and add need to properly apply the secret bit masking magic. So, I'd suggest writing a follow-on patch to turn SymbolTable into a true class.
(In reply to comment #9) > This patch looks good to me, with one caveat: I'd really like to see > SymbolTable become a true class that has a HashMap data member, and does the > bit masking for you automatically. Right now, SymbolTable is kind of > error-prone, since all clients of get, set, and add need to properly apply the > secret bit masking magic. Yeah, I asked in my original request for review for the best way to remove that problem. I thought about it a bit last night, and came to the same solution you proposed, so that's probably best. > So, I'd suggest writing a follow-on patch to turn SymbolTable into a true > class. I might as well do it before I ask anyone to land it for me.
> > So, I'd suggest writing a follow-on patch to turn SymbolTable into a true > > class. > > I might as well do it before I ask anyone to land it for me. OK, I'll clear the review bit and hold off on landing for now.
On cue, this patch is now a 0.3% regression. I don't think it's real because of the progression before.
Created attachment 20964 [details] Revised proposed patch Here is a revised version of my patch, using the attributes support added in r32859. I didn't think that approach would have good enough performance, but thankfully it seems to be fine. This doesn't change any JavaScriptCore test results, and I couldn't actual run the layout tests, because r32859 broke JS in an actual browser window, but the const tests work fine when I run them on the command line. After the tests are fixed, I'll run them and ask for review.
Comment on attachment 20964 [details] Revised proposed patch The layout tests are now fixed, and this patch indeed fixes the const tests.
Committed r32866