[webkit-reviews] review denied: [Bug 10701] [ES5] Implement strict mode : [Attachment 69578] Patch

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Oliver Hunt
<oliver at apple.com>'s request for review:
Bug 10701: [ES5] Implement strict mode
https://bugs.webkit.org/show_bug.cgi?id=10701

Attachment 69578: Patch
https://bugs.webkit.org/attachment.cgi?id=69578&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=69578&action=review

> JavaScriptCore/interpreter/Interpreter.cpp:378
> +	   // FIXME: We can use the preparser in strict mode, we just need
additional logic

Would be really great to file a bug for this, and reference the bug number in
the comment!

> JavaScriptCore/parser/JSParser.cpp:259
> +	   void pushLabel(const Identifier* label)

probably should have a newline before function definition.

> JavaScriptCore/parser/JSParser.cpp:265
> +	   void popLabel()

probably should have a newline before function definition.

> JavaScriptCore/parser/JSParser.cpp:271
> +	   bool hasLabel(const Identifier* label)

probably should have a newline before function definition.

> JavaScriptCore/runtime/Arguments.cpp:203
> +	   createStrictModeCalleeIfNecessary(exec);

You seem to be checking d->overrodeCallee twice?
Since we only createStrictModeCalleeIfNecessary this if !d->overrodeCallee, we
should only need an ASSERT within the function?

Agreed to all Sam's comments.
Three more issues:

(1) failIfStrictTrue/failIfStrictFalse.

I find these names a little confusing.	Strict is usually a modifier to the
thing it precedes, e.g. "strict equal".  I think something like
"strictModeFailIfFalse" or "failIfFalseIfStrict" would parse in a more
understandably fashion for me.

(2) Performance.

Given the size of this change and the additional parameterization in all the
'put' methods I think this bug really needs before and after SunSpidey & v8
numbers.  (We should also probably also have numbers for the interpreter -
maybe just for SunSpider - to at least be aware in advance of any impact
there).

(3) Passing exec through reparseExceptionInfo/parse/jsParse/parseProgram.

We really shouldn't be pushing a pointer into the JS Stack this deep into the
parser – and we really shouldn't need to.  We want to be moving in the other
direction – paring back our use of ExecState, to places where we may actually
trigger new execution.	It looks like you've passed the exec state to
parseProgram because it needs to check for the presence of certain properties
on the LGO? If so, we should have an appropriate hasProperty method that does
not require an exec state (and if we don't have one, I'd think you should be
able to add one that just wraps the getPropertySlot that you're calling,
passing the globalExec from the LGO).  Did I miss a use of ExecState that
really requires a JS stack? – if not, I think we need to revert this.

r- for the JSGlobalData* -> ExecState* change, that makes me too sad. :'-( :-P

All looks great otherwise!
G.