1126552 - Make nsTArray support range-based for loop

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

We have been allowed to use range-based for loop in our codebase, but as the most widely-used array type, nsTArray, does not support such thing, we cannot use it in most places. We need to support it.

I think the simplest way is to add begin() and end() methods to nsTArray_Impl.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: example

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8555529 [details] [diff] [review]
patch

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

::: xpcom/glue/nsTArray.h
@@ +990,5 @@
>    {
>      return SafeElementAt(Length() - 1, aDef);
>    }
>  
> +  // Methods for "range-based for loop"

Nit on the phrasing: "Methods for range-based for loops."  (Trailing period included in the comment.)

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

I guess it would be better to have a typedef for the iterator.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch 0 - avoid shadow warnings

This patch is for avoiding warnings like https://treeherder.mozilla.org/logviewer.html#?job_id=4644032&repo=try

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7e5f52141c5

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8558304 [details] [diff] [review]
patch 0 - avoid shadow warnings

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

Sigh, -Wshadow.

::: xpcom/glue/nsTArray.h
@@ +1026,5 @@
>    template<class Item, class Comparator>
>    index_type IndexOf(const Item& aItem, index_type aStart,
>                       const Comparator& aComp) const
>    {
> +    const elem_type* iter = Elements() + aStart, *iend = Elements() + Length();

While you're here, would you mind splitting these up:

const elem_type* iter = ...;
const elem_type* iend = ...;

Or I suppose you could declare them as:

const_iterator iter = ..., iend = ...;

Whichever one you like.  If you wanted to open a separate bug for cleaning the declarations up, that'd be OK too.

@@ +1059,5 @@
>    index_type LastIndexOf(const Item& aItem, index_type aStart,
>                           const Comparator& aComp) const
>    {
>      size_type endOffset = aStart >= Length() ? Length() : aStart + 1;
> +    const elem_type* iend = Elements() - 1, *iter = iend + endOffset;

Likewise.

@@ +1540,5 @@
>        return nullptr;
>      }
>  
>      // Initialize the extra array elements
> +    elem_type* iter = Elements() + aIndex, *iend = iter + aCount;

Likewise.

@@ +1565,5 @@
>        return nullptr;
>      }
>  
>      // Initialize the extra array elements
> +    elem_type* iter = Elements() + aIndex, *iend = iter + aCount;

Likewise.

@@ +1695,5 @@
>    // @param aStart The index of the first element to destroy.
>    // @param aCount The number of elements to destroy.
>    void DestructRange(index_type aStart, size_type aCount)
>    {
> +    elem_type* iter = Elements() + aStart, *iend = iter + aCount;

Likewise.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c987e8202892

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28dadab99ddf
https://hg.mozilla.org/integration/mozilla-inbound/rev/6786f92cb4d6

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/28dadab99ddf
https://hg.mozilla.org/mozilla-central/rev/6786f92cb4d6