775166 - Remove some ugly optimization in jsarray

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S. (please needinfo tschuster)]

Attachment: v1

This seems very premature to optimize, because this is unlikely to really matter. Checking if something is a hole based on whether this index was atomized before seems really ugly.

Comment 1

[Brian Hackett [Laid off!]]

Comment on attachment 643435 [details] [diff] [review]
v1

Review of attachment 643435 [details] [diff] [review]:
-----------------------------------------------------------------

Wow, gross.

Comment 2

[Tom S. (please needinfo tschuster)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/60b949c0eaef

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/60b949c0eaef

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 643435 [details] [diff] [review]
v1

Review of attachment 643435 [details] [diff] [review]:
-----------------------------------------------------------------

I was always pretty meh about this ugly, nice to see someone caring more than me enough to remove it.  :-)  A few little comments in passing, while I'm skimming the bugmail backlog.

::: js/src/jsarray.cpp
@@ +260,5 @@
>      return true;
>  }
>  
> +bool
> +DoubleIndexToId(JSContext *cx, double index, jsid *id)

Shouldn't this be static bool?

@@ +261,5 @@
>  }
>  
> +bool
> +DoubleIndexToId(JSContext *cx, double index, jsid *id)
> +{

I think this method can assert that index is finite and a non-negative integer, right?

@@ +262,5 @@
>  
> +bool
> +DoubleIndexToId(JSContext *cx, double index, jsid *id)
> +{
> +    if (index == uint32_t(index))

Tom, if |index|, truncated, isn't a uint32_t, this invokes undefined behavior.  I *think*, if I remember correctly, that this method's only called with integral values, so couldn't you change this test to |index <= JSID_INT_MAX| to avoid this problem?