Add a `reverse` method to `mozilla::Vector`

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
Created attachment 8718076 [details] [diff] [review]
Add a `reverse` method to `mozilla::Vector`
Attachment #8718076 - Flags: review?(jwalden+bmo)
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb2ed747e9f2
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Blocks: 961323

Comment 3

3 years ago
Comment on attachment 8718076 [details] [diff] [review]
Add a `reverse` method to `mozilla::Vector`

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

::: mfbt/Vector.h
@@ +517,5 @@
>    ConstRange all() const { return ConstRange(begin(), end()); }
>  
>    /* mutators */
>  
> +  void reverse() {

Add

  MOZ_REENTRANCY_GUARD_ET_AL;

since as documented earlier none of these methods are reentrant.

And given that, we can slightly optimize this to avoid massive over-repetition of begin() here.

@@ +521,5 @@
> +  void reverse() {
> +    for (size_t i = 0; i < length() / 2; i++) {
> +      T temp(Move(begin()[i]));
> +      begin()[i] = Move(begin()[length() - i - 1]);
> +      begin()[length() - i - 1] = Move(temp);

How about using mozilla::Swap instead of the manual local temp?

So then you'd have

  T* elems = begin();
  size_t len = length();
  for (size_t i = 0, mid = len / 2; i < mid; i++) {
    Swap(elems[i], elems[len - i - 1]);
  }

::: mfbt/tests/TestVector.cpp
@@ +167,5 @@
>  
> +void
> +mozilla::detail::VectorTesting::testReverse()
> +{
> +  Vector<UniquePtr<uint8_t>> vec;

Just for general sanity, could you test this on a Vector storing elements only in reserved space, and on a Vector storing elements in allocated space?  Also test an empty array and an array with an even number of elements, to test the floordiv in the implementation doesn't have off-by-one issues.
Attachment #8718076 - Flags: review?(jwalden+bmo) → review+
Created attachment 8718374 [details] [diff] [review]
Add a `reverse` method to `mozilla::Vector`
Attachment #8718374 - Flags: review+
Attachment #8718076 - Attachment is obsolete: true
Keywords: checkin-needed

Comment 7

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d7264fe9b1a1
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.