Closed Bug 1445302 Opened 2 years ago Closed 2 years ago

Replace TArray.RemoveElementAt(TArray.Length() - 1) pattern with TArray.RemoveLastElement() or TArray.PopLastElement()

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: miko, Assigned: miko)

References

Details

Attachments

(1 file)

This patch replaces the calls in the form of TArray.RemoveElementAt(TArray.Length() - 1) with TArray.RemoveLastElement(), or if it is possible and trivial, with TArray.PopLastElement().
Comment on attachment 8958506 [details]
Bug 1445302 - Replace TArray.RemoveElementAt(TArray.Length() - 1) pattern with TArray.RemoveLastElement() or TArray.PopLastElement()

https://reviewboard.mozilla.org/r/227432/#review233248

Thank you for doing this.  Some comments/questions below.

::: gfx/gl/GLTextureImage.cpp:461
(Diff revision 1)
>      unsigned int length = mImages.Length();
>      for (; i < length; i++)
> -      mImages.RemoveElementAt(mImages.Length()-1);
> +      mImages.RemoveLastElement();

Wonder if it's worth making this `mImages.RemoveElementsAt(i, /* some weird count computation */);`.  WDYT?

::: gfx/vr/gfxVRPuppet.cpp:746
(Diff revision 1)
>    while (mPuppetHMDs.Length() > mPuppetDisplayCount) {
> -    mPuppetHMDs.RemoveElementAt(mPuppetHMDs.Length() - 1);
> +    mPuppetHMDs.RemoveLastElement();
>    }

Same thing here for using `RemoveElementsAt`.

::: image/DecodePool.cpp:230
(Diff revision 1)
>  
>    Work PopWorkFromQueue(nsTArray<RefPtr<IDecodingTask>>& aQueue)
>    {
>      Work work;
>      work.mType = Work::Type::TASK;
> -    work.mTask = aQueue.LastElement().forget();
> +    work.mTask = aQueue.PopLastElement().forget();

I think you can actually forego the `.forget()` call.  Since `PopLastElement()` will return a temporary `RefPtr` here, the move constructor will be used automatically, which avoids refcounting, just like the `.forget()` call was doing originally.

::: image/imgLoader.cpp:1074
(Diff revision 1)
>    RefPtr<imgCacheEntry> entry = Move(mQueue.LastElement());
> -  mQueue.RemoveElementAt(mQueue.Length() - 1);
> +  mQueue.RemoveLastElement();

Isn't this just:

`RefPtr<...> entry = mQueue.PopLastElement();`

?

::: ipc/mscom/Utils.cpp:343
(Diff revision 1)
>      RefPtr<ITypeInfo> curTypeInfo(Move(typeInfos.LastElement()));
> -    typeInfos.RemoveElementAt(typeInfos.Length() - 1);
> +    typeInfos.RemoveLastElement();

Same question here for using `PopLastElement`.

::: layout/painting/RetainedDisplayListBuilder.cpp:196
(Diff revision 1)
>    if (aIndex != (aArray.Length() - 1)) {
>      T last = aArray.LastElement();
>      aArray.LastElement() = aArray[aIndex];
>      aArray[aIndex] = last;
>    }
>  
> -  aArray.RemoveElementAt(aArray.Length() - 1);
> +  aArray.RemoveLastElement();

I think all this code can just be `UnorderedRemoveElementAt` now, but that should probably be a followup bug.
Attachment #8958506 - Flags: review?(nfroyd) → review+
Comment on attachment 8958506 [details]
Bug 1445302 - Replace TArray.RemoveElementAt(TArray.Length() - 1) pattern with TArray.RemoveLastElement() or TArray.PopLastElement()

https://reviewboard.mozilla.org/r/227432/#review233248

> Wonder if it's worth making this `mImages.RemoveElementsAt(i, /* some weird count computation */);`.  WDYT?

This might change the intended behavior, as removing elements would start from the beginning index and not the end.

> Same thing here for using `RemoveElementsAt`.

Same reasoning as above.

> I think all this code can just be `UnorderedRemoveElementAt` now, but that should probably be a followup bug.

This code is deprecated and will get removed (bug 1436409).
Pushed by mikokm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bceb565cfe14
Replace TArray.RemoveElementAt(TArray.Length() - 1) pattern with TArray.RemoveLastElement() or TArray.PopLastElement() r=froydnj
https://hg.mozilla.org/mozilla-central/rev/bceb565cfe14
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.