Remove some ugly optimization in jsarray

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

Trunk
mozilla17
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment)

Created attachment 643435 [details] [diff] [review]
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.
Attachment #643435 - Flags: review?(bhackett1024)
Comment on attachment 643435 [details] [diff] [review]
v1

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

Wow, gross.
Attachment #643435 - Flags: review?(bhackett1024) → review+
Whiteboard: [js:t]
http://hg.mozilla.org/integration/mozilla-inbound/rev/60b949c0eaef
https://hg.mozilla.org/mozilla-central/rev/60b949c0eaef
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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?
You need to log in before you can comment on or make changes to this bug.