Issue 9696035: Function declarations shall not overwrite read-only global properties.

Issue 9696035: Function declarations shall not overwrite read-only global properties. (Closed)

Created:
8 years, 9 months ago by rossberg

Modified:
8 years, 9 months ago

Reviewers:
Michael Starzinger

CC:
v8-dev

Base URL:
https://v8.googlecode.com/svn/branches/bleeding_edge

Visibility:
Public.

More Reviews

Description

Function declarations shall not overwrite read-only global properties. R=mstarzinger@chromium.org BUG=115452 TEST=mjsunit/regress/regress-115452 Committed: https://code.google.com/p/v8/source/detail?r=11043

Patch Set 2 : Addressed Michael's comments. #

Created: 8 years, 9 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+63 lines, -6 lines)			Patch
	M	src/runtime.cc	View		3 chunks	+15 lines, -6 lines	0 comments	Download
	A	test/mjsunit/regress/regress-115452.js	View	1	1 chunk	+48 lines, -0 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Michael Starzinger

LGTM (with nits). https://chromiumcodereview.appspot.com/9696035/diff/1/test/mjsunit/regress/regress-115452.js File test/mjsunit/regress/regress-115452.js (right): https://chromiumcodereview.appspot.com/9696035/diff/1/test/mjsunit/regress/regress-115452.js#newcode37 test/mjsunit/regress/regress-115452.js:37: assertEquals(1, this.foobl); Better use assertSame() here. ...

8 years, 9 months ago (2012-03-14 13:38:07 UTC) #2

rossberg

8 years, 9 months ago (2012-03-14 13:44:13 UTC) #3

https://chromiumcodereview.appspot.com/9696035/diff/1/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-115452.js (right):

https://chromiumcodereview.appspot.com/9696035/diff/1/test/mjsunit/regress/re...
test/mjsunit/regress/regress-115452.js:37: assertEquals(1, this.foobl);
On 2012/03/14 13:38:07, Michael Starzinger wrote:
> Better use assertSame() here.

Done.

https://chromiumcodereview.appspot.com/9696035/diff/1/test/mjsunit/regress/re...
test/mjsunit/regress/regress-115452.js:42: assertEquals(1, this.foobl);
On 2012/03/14 13:38:07, Michael Starzinger wrote:
> Likewise.

Done.

https://chromiumcodereview.appspot.com/9696035/diff/1/test/mjsunit/regress/re...
test/mjsunit/regress/regress-115452.js:46: eval("function foobl() {}");
On 2012/03/14 13:38:07, Michael Starzinger wrote:
> Why do you test this twice? Isn't once sufficient?

You would think so, but you never know whether the first one somehow affects
something in the background that would make the second succeed. :)

Expand Messages | Collapse Messages