Jan de Mooij [:jandem] Assignee
	Description • 8 years ago

I noticed this in bug 1306450. It's silly: we were doing a GETELEM on a typed array, but we first attached a dense stub for no good reason (typed arrays can't have dense elements).

	Hannes Verschore [:h4writer]
	Comment 1 • 8 years ago

Comment on attachment 8796562 [details] [diff] [review]
Patch

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

Good catch!

::: js/src/jit/IonCaches.cpp
@@ +3827,5 @@
>      if (!obj->isNative() || !idval.isInt32())
>          return true;
>  
> +    if (uint32_t(idval.toInt32()) >= obj->as<NativeObject>().getDenseInitializedLength())
> +        return true;

Shouldn't we split this into more verbose parts? I know it is the same, but it is easier to see all reasons, instead of just hiding it in one line?

if (idval.toInt32() < 0)
    return true;

if (obj->as<NativeObject>().getDenseInitializedLength() == 0)
    return true;

if (idval.toInt32() >= obj->as<NativeObject>().getDenseInitializedLength())
    return true;

	Jan de Mooij [:jandem] Assignee
	Comment 2 • 8 years ago

(In reply to Hannes Verschore [:h4writer] from comment #1)
> Shouldn't we split this into more verbose parts? I know it is the same, but
> it is easier to see all reasons, instead of just hiding it in one line?

Well it's just a bounds check, I think we have many places where we do a single compare like this one.

	Hannes Verschore [:h4writer]
	Comment 3 • 8 years ago

(In reply to Jan de Mooij [:jandem] from comment #2)
> (In reply to Hannes Verschore [:h4writer] from comment #1)
> > Shouldn't we split this into more verbose parts? I know it is the same, but
> > it is easier to see all reasons, instead of just hiding it in one line?
> 
> Well it's just a bounds check, I think we have many places where we do a
> single compare like this one.

Okay, r+ either way.

	Pulsebot
	Comment 4 • 8 years ago

Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7cb15d415ee
Don't attach an Ion GetDenseElement stub if we're not accessing a dense element. r=h4writer

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 5 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b7cb15d415ee