950701 - Use Vector's API more

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: vector-tidyings.patch

Attached is a patch which makes better use of Vector's API, replacing things like size() == 0 with empty(), and x[x.length() - 1] with x.back(), etc.

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 8348077 [details] [diff] [review]
vector-tidyings.patch

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

I am reminded of initial (when Vector was first landed) thoughts that absence of an "is" prefix on "empty" would be better -- on the theory that adjective-named methods should stand alone as such.  Then after a little extra time using it, people began to think that might be wrong at least as concerned "empty".  Perhaps because of its also reading as a verb.  I wonder, reading a whole bunch of confusing "empty" usage here, if we shouldn't actually revisit that initial choice, and actually do a rename.

::: js/src/jit/LIR.cpp
@@ +42,5 @@
>  bool
>  LIRGraph::noteNeedsSafepoint(LInstruction *ins)
>  {
>      // Instructions with safepoints must be in linear order.
> +    JS_ASSERT_IF(safepoints_.length(), safepoints_.back()->id() < ins->id());

Seems like !safepoints_.empty() should be here, too.

Comment 2

[Dan Gohman [:sunfish]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1)
> Comment on attachment 8348077 [details] [diff] [review]
> vector-tidyings.patch
> 
> Review of attachment 8348077 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am reminded of initial (when Vector was first landed) thoughts that
> absence of an "is" prefix on "empty" would be better -- on the theory that
> adjective-named methods should stand alone as such.  Then after a little
> extra time using it, people began to think that might be wrong at least as
> concerned "empty".  Perhaps because of its also reading as a verb.  I
> wonder, reading a whole bunch of confusing "empty" usage here, if we
> shouldn't actually revisit that initial choice, and actually do a rename.

I can see advantages both to being consistent with std::vector and to being consistent with the coding style in the tree. I don't have a strong opinion about what's better here.

> ::: js/src/jit/LIR.cpp
> @@ +42,5 @@
> >  bool
> >  LIRGraph::noteNeedsSafepoint(LInstruction *ins)
> >  {
> >      // Instructions with safepoints must be in linear order.
> > +    JS_ASSERT_IF(safepoints_.length(), safepoints_.back()->id() < ins->id());
> 
> Seems like !safepoints_.empty() should be here, too.

Fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f75c03ae992c

Comment 3

[Carsten Book [:Tomcat]]

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