Status

()

enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

unspecified
mozilla29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

6 years ago
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.
Attachment #8348077 - Flags: review?(jwalden+bmo)
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.
Attachment #8348077 - Flags: review?(jwalden+bmo) → review+
Assignee

Comment 2

6 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/f75c03ae992c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.