Make nsTArray support range-based for loop

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

Trunk
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.
Posted patch patch (obsolete) — Splinter Review
Attachment #8555529 - Flags: review?(nfroyd)
Posted patch exampleSplinter Review
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.)
Attachment #8555529 - Flags: review?(nfroyd) → review+
Posted patch patch (obsolete) — Splinter Review
I guess it would be better to have a typedef for the iterator.
Assignee: nobody → quanxunzhen
Attachment #8555529 - Attachment is obsolete: true
Attachment #8556093 - Flags: review?(nfroyd)
Posted patch patchSplinter Review
Attachment #8556093 - Attachment is obsolete: true
Attachment #8556093 - Flags: review?(nfroyd)
Attachment #8556105 - Flags: review?(nfroyd)
Attachment #8556105 - Flags: review?(nfroyd) → review+
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.
Attachment #8558304 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/28dadab99ddf
https://hg.mozilla.org/mozilla-central/rev/6786f92cb4d6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1299489
You need to log in before you can comment on or make changes to this bug.