Thanks for the review, Lasse, I applied all changes. Because passing the
enum
instead of bool around touches a lot of places (same places I already
modified
but nonetheless) I am not committing just yet and will wait for you to take
another look, if you like.
I plan to commit tomorrow unless I hear otherwise.
Thank you!
Martin
http://codereview.chromium.org/6286043/diff/1/src/runtime.cc
File src/runtime.cc (right):
http://codereview.chromium.org/6286043/diff/1/src/runtime.cc#newcode7557
src/runtime.cc:7557: false);
On 2011/02/03 12:10:41, Lasse Reichstein wrote:
Passing too many boolean literals (some say one is too many) makes the
code hard
to read. It's not obvious what "true" and "false" means here.
At least name the argument here, e.g.,
bool is_strict = false;
... ::CompileEval(source, context, true, is_strict);
or better yet, introduce an enum type for code strictness with kStrict
and
kNonStrict as members, and use that to pass around strictness
parameters.
Done.
http://codereview.chromium.org/6286043/diff/1/test/mjsunit/strict-mode-eval.js
File test/mjsunit/strict-mode-eval.js (right):
http://codereview.chromium.org/6286043/diff/1/test/mjsunit/strict-mode-eval.js#newcode31
test/mjsunit/strict-mode-eval.js:31: var no_exc = true;
On 2011/02/03 12:10:41, Lasse Reichstein wrote:
is "exc" short for "exception"?
I typically see "exn" for that (not that it's much better, just spell
it all
out).
Done.
http://codereview.chromium.org/6286043/diff/1/test/mjsunit/strict-mode-eval.js#newcode34
test/mjsunit/strict-mode-eval.js:34: no_exc = false;
On 2011/02/03 12:10:41, Lasse Reichstein wrote:
Just use
assertUnreachable("did not throw exception")
here.
Done.
http://codereview.chromium.org/6286043/diff/1/test/mjsunit/strict-mode-eval.js#newcode53
test/mjsunit/strict-mode-eval.js:53: eval("var x = '\\020;");
The nested functions exercise the lazy compilation. Strictly speaking
the 4 levels are not necessary but it tests yet another facet of the
code.
On 2011/02/03 12:10:41, Lasse Reichstein wrote:
The string looks unterminated. You are probably not getting the
octal-escape
SyntaxError.
Perhaps make check for each argument to eval that it is not an error
in
non-strict mode.
Why the nested functions?
http://codereview.chromium.org/6286043/diff/1009/src/compiler.h
File src/compiler.h (right):
http://codereview.chromium.org/6286043/diff/1009/src/compiler.h#newcode168
src/compiler.h:168: class IsStrict: public BitField<bool, 4, 1> {};
On 2011/02/03 12:36:01, Lasse Reichstein wrote:
Move it below IsInLoop, to keep bits in order.
Done. Added comment to clarify that IsStrict is used in eager
compilation (that was originally the motivation for the out of order
bits).
http://codereview.chromium.org/6286043/diff/1009/src/objects.cc
File src/objects.cc (right):
http://codereview.chromium.org/6286043/diff/1009/src/objects.cc#newcode7993
src/objects.cc:7993: bool strict = Smi::cast(pair->get(2))->value() !=
0;
On 2011/02/03 12:36:01, Lasse Reichstein wrote:
Do you need to store the strictness in a separate field? Could we
store it on
the SharedFunctionInfo instead?
I believe I do need to store the strict flag. The strict is stored on
the SharedFunctionInfo but the SharedFunctionInfo is not always the info
of the immediately calling JSFunction. The canonical counter-example is:
"use strict";
eval("x = 1");
When we call into the Runtime_ResolvePossiblyDirectEval I do:
StackFrameLocator locator;
JavaScriptFrame* frame = locator.FindJavaScriptFrame(0);
And observe that:
Top::context()->closure() != frame->function()
and moreover:
Top::context()->closure()->shared()->strict_mode() == false
whereas:
((JSFunction*)(frame->function()))->shared()->strict_mode() == true
So the SharedFunctionInfo we have available here as a key into the eval
cache is not sufficient piece of information to key off of ... so I have
to store the strict mode flag in the key explicitly.
Unless the disparity I describe above is symptom of some deeper problem
in the system.
http://codereview.chromium.org/6286043/diff/1009/src/objects.cc#newcode8011
src/objects.cc:8011: hash ^= strict ? 0x8000 : 0x800;
On 2011/02/03 12:36:01, Lasse Reichstein wrote:
I don't think it matters what you do for strict-mode hash, since we
are unlikely
to have the same code as both strict and non-strict. I wouldn't mind a
hash
collision in that case (but xoring by 0x8000 (or anything non-zero) in
one case
is fine, I don't see why you xor by something non-zero in the other
case).
Done.
http://codereview.chromium.org/6286043/diff/1009/src/objects.cc#newcode9006
src/objects.cc:9006: StringSharedKey key(src,
context->closure()->shared(), is_strict);
On 2011/02/03 12:36:01, Lasse Reichstein wrote:
I'm not sure I understand the question.
You can have many JSFunctions created using the same
SharedFunctionInfo. The
SharedFunctionInfo corresponds to the function literal in the source,
so it
identifies the function code as well as the closure that it has access
to.
If a two functions have the same SharedFunctionInfo, they will be able
to use
the same code, because they run in the same environment.
Makes sense, thanks for the explanation.
http://codereview.chromium.org/6286043/diff/1009/src/objects.h
File src/objects.h (right):
http://codereview.chromium.org/6286043/diff/1009/src/objects.h#newcode4175
src/objects.h:4175: inline void set_strict_mode(bool value);
On 2011/02/03 12:36:01, Lasse Reichstein wrote:
Is the argument needed? I.e., do we ever set a function back from
strict mode to
non-strict mode?
Strictly speaking, the argument is not needed (assuming that GC heap
allocated objects are zeroed out by the memory allocator).
Function never transitions strict->non-strict. However, where the
SharedFunctionInfo is being set up, all flags on shared info are set in
similar way:
function_info_>set_xxx_xxx(lit->xxx_xxx());
So the shared function info transitions from "newly created" to either
strict or non-strict.
It felt cleaner to keep the same code pattern across the board than have
strict mode behaving differently than all the other flags. I am open to
changing this although this version of code has the high degree of
consistency going for it.
http://codereview.chromium.org/6286043/diff/1009/src/parser.cc
File src/parser.cc (right):
http://codereview.chromium.org/6286043/diff/1009/src/parser.cc#newcode755
src/parser.cc:755: }
On 2011/02/03 12:36:01, Lasse Reichstein wrote:
Is it only for lazily compiled functions that the SharedFunctionInfo
holds the
strict-mode, or is it all SharedFunctionInfo?
All SharedFunctionInfo have the correct strict mode set (unless I have
bug somewhere but I tried my best to ensure that all SharedFunctionInfo
have the strict mode flag set). Whenever SharedFunctionInfo is created
the strict mode flag is copied to it from the FunctionLiteral.
http://codereview.chromium.org/6286043/diff/1009/src/runtime.cc
File src/runtime.cc (right):
http://codereview.chromium.org/6286043/diff/1009/src/runtime.cc#newcode9816
src/runtime.cc:9816: false);
On 2011/02/03 12:36:01, Lasse Reichstein wrote:
It deserves a comment that we always run the code as non-strict, even
in a
strict code context.
Done.
http://codereview.chromium.org/6286043/
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
