On 2014/02/02 19:59:56, sof wrote:
https://codereview.chromium.org/118553003/diff/170001/test/mjsunit/harmony/symbols.js
File test/mjsunit/harmony/symbols.js (right):
https://codereview.chromium.org/118553003/diff/170001/test/mjsunit/harmony/symbols.js#newcode172
test/mjsunit/harmony/symbols.js:172: assertEquals(Object(symbols[i]),
Object(symbols[i]))
On 2014/02/02 19:49:35, arv wrote:
> On 2014/02/01 13:58:10, sof wrote:
> > On 2014/02/01 01:33:10, arv wrote:
> > > This looks strange. If symbol[i] is a symbol (not a symbol wrapper)
then
> this
> > > should be false. Are all elements in symbols wrappers? It does not
seem
like
> > > that is the case on line 49.
> >
> > They're all Symbol values. This is an equals check, not same, so why
should
> > deepObjectEquals() return false? (Perhaps not a very interesting to
test
to
> add,
> > I can remove.)
>
> If all elements in symbols are symbol values (not symbol wrapper
objects)
this
> should be assertNotEquals since a new wrapper is always created when
you do
> Object(value).
I know; the object also doesn't have any "keys" in the (Object.keys()
sense),
so
I don't think it much of a difference how you want to phrase that equality
check
for these wrapper objects. Had assertNotEquals existed, that is.
The offer to remove the test still stands; I don't think there is anything
fundamentally wrong here in the interpretation of symbol values vs
wrappers.
I see, assertEquals is just broken. For other wrappers it unwraps which is
clearly wrong. It also trats two completely different object as equal.
Yikes.
Can you change the test to?
assertTrue(Object(symbol[i]) !== Object(symbol[i]));
or add assertNotSame and use that?
https://codereview.chromium.org/118553003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
