Reversely iterating nsTArray

[Xidorn Quan]

I asked a question in #developers that what is the best way to reversely
iterating nsTArray, and there are some suggestions:

<tbsaunde> uint32_t count = array.Length(); for (uint32_t i = length - 1; i
< length; i--)
<smaug> iterate from length() to 1 and index using i - 1
<jcranmer> for (uint32_t i = array.Length(); i-- > 0; ) { }

tbsaunde's method should work fine, but not intuitive. smaug's looks fine
to me, but could cause some problem if I use i instead of i-1 by mistake.
jcranmer's... I don't think it is a good idea, anyway. None of them looks
prefect to me.


As we have supported range-based for loop in our code, I purpose that we
have something like ReverseIntegerRange, and use this with range-based loop
to iterate the index. So we can use, for example: for (auto i :
ReverseIntegerRange(array.Length()))

Maybe we can also add IntegerRange to benefit from the integer type
dedution.

How does it sound?

- Xidorn

[Seth Fowler]

Sounds good! +1 from me.

Bike shedding:

- Make Range() and ReverseRange() templates, so you can use them with any type that supports the appropriate operators. This also implies removing ‘Integer’ from their names, I think.

- It’d be nice to add a constructor that supports specifying both the beginning and the end points, and another constructor that further supports specifying a stride.

- Seth

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Xidorn Quan]

On Wed, Jan 28, 2015 at 3:18 PM, Seth Fowler <se...@mozilla.com> wrote:

> Sounds good! +1 from me.
>
> Bike shedding:
>
> - Make Range() and ReverseRange() templates, so you can use them with any
> type that supports the appropriate operators. This also implies removing
> ‘Integer’ from their names, I think.
>

I wanted to use this name as well, but there is one problem that we have
had mozilla::Range template in MFBT, which is for pointers. I guess we
probably can rename it to something like PointerRange.

- Xidorn

[Kyle Huey]

On Wed, Jan 28, 2015 at 1:18 PM, Seth Fowler <se...@mozilla.com> wrote:

> Sounds good! +1 from me.
>
> Bike shedding:
>
> - Make Range() and ReverseRange() templates, so you can use them with any
> type that supports the appropriate operators. This also implies removing
> ‘Integer’ from their names, I think.
>

> - It’d be nice to add a constructor that supports specifying both the
> beginning and the end points, and another constructor that further supports
> specifying a stride.

YAGNI*. Someone can implement those once they need them.

- Kyle

* "You aren't gonna need it"

[Seth Fowler]

Fiddlesticks! I’d have immediate use for both of those things, that’s why I suggested them.

Also, the first suggestion is probably actually less typing than not doing it that way. You’re basically writing ‘template <typename T>’ at the front, where you lose some characters, but then you make it up every time you type ’T’ instead of ‘uint32_t’ or whatever you’d have used otherwise. =)

- Seth

> On Jan 27, 2015, at 11:47 PM, Kyle Huey <m...@kylehuey.com> wrote:

[Cameron McCormack]

Xidorn Quan:

> I asked a question in #developers that what is the best way to reversely
> iterating nsTArray, and there are some suggestions:

For cases where we don’t need to know the index of the array, can we
support something like:

for (e : array.ReverseIterator()) { ... }

or:

for (e : ReverseIterator(array)) { ... }

(I notice that boost::adaptors::reversed is something like this.)

There’s a danger that it’s not clear what happens if you modify the
array while using the iterator, but it’s probably no worse than using
the indexes that would come from ReverseIntegerRange.

--
Cameron McCormack ≝ http://mcc.id.au/

[Xidorn Quan]

Hmm. It reminds me that we might not need a ReverseIntegerRange at all. We
can have a Range, and a Reversed, then combine them to achieve
ReversedRange. We can make Reversed works with nsTArray as well :)

- Xidorn

[Seth Fowler]

Yes, that’s a good idea! I like this even better than the original proposal.

- Seth

> On Jan 28, 2015, at 1:59 AM, Xidorn Quan <quanx...@gmail.com> wrote:

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org <mailto:dev-pl...@lists.mozilla.org>
> https://lists.mozilla.org/listinfo/dev-platform <https://lists.mozilla.org/listinfo/dev-platform>

[Ehsan Akhgari]

On 2015-01-27 9:24 PM, Xidorn Quan wrote:
> I asked a question in #developers that what is the best way to reversely
> iterating nsTArray, and there are some suggestions:
>

> <tbsaunde> uint32_t count = array.Length(); for (uint32_t i = length - 1; i
> < length; i--)
> <smaug> iterate from length() to 1 and index using i - 1
> <jcranmer> for (uint32_t i = array.Length(); i-- > 0; ) { }
>
> tbsaunde's method should work fine, but not intuitive. smaug's looks fine
> to me, but could cause some problem if I use i instead of i-1 by mistake.
> jcranmer's... I don't think it is a good idea, anyway. None of them looks
> prefect to me.
>
>
> As we have supported range-based for loop in our code, I purpose that we
> have something like ReverseIntegerRange, and use this with range-based loop
> to iterate the index. So we can use, for example: for (auto i :
> ReverseIntegerRange(array.Length()))
>
> Maybe we can also add IntegerRange to benefit from the integer type
> dedution.
>
> How does it sound?

No, please don't do this. We need to make our container classes more
similar to the STL containers, not less, and since this is about adding
new functionality, here is what we need to do:

* Add a begin() and end() function to nsTArray that return iterators
with STL semantics.
* Add a rbegin() and rend() function to nsTArray with similar semantics
which return a reverse iterator.
* Add something similar to boost::adaptors::reverse, probably to MFBT.

That way you can write:

for (auto& elem : array)

or:

for (auto& elem : mozilla::Reverse(array))

And that would work no matter whether array is an nsTArray or a std::vector.

[Xidorn Quan]

On Thu, Jan 29, 2015 at 2:59 AM, Ehsan Akhgari <ehsan....@gmail.com>
wrote:

That's exactly what I want to do :)

And we still need a Range class for iterating integer.

- Xidorn

[Xidorn Quan]

> That way you can write:
>
> for (auto& elem : array)
>
> or:
>
> for (auto& elem : mozilla::Reverse(array))
>
> And that would work no matter whether array is an nsTArray or a
> std::vector.
>

Ah... It seems that, to achieve this, we have to copy almost the whole
iterator concept from the STL, because it seems that <iterator> cannot be
safely included directly, as well as a large part of range concept from the
Boost. It looks too much to do... Any suggestion?

- Xidorn

[Trevor Saunders]

On Wed, Jan 28, 2015 at 10:59:48AM -0500, Ehsan Akhgari wrote:
> On 2015-01-27 9:24 PM, Xidorn Quan wrote:
> >I asked a question in #developers that what is the best way to reversely
> >iterating nsTArray, and there are some suggestions:
> >
> ><tbsaunde> uint32_t count = array.Length(); for (uint32_t i = length - 1; i
> >< length; i--)
> ><smaug> iterate from length() to 1 and index using i - 1
> ><jcranmer> for (uint32_t i = array.Length(); i-- > 0; ) { }
> >
> >tbsaunde's method should work fine, but not intuitive. smaug's looks fine
> >to me, but could cause some problem if I use i instead of i-1 by mistake.
> >jcranmer's... I don't think it is a good idea, anyway. None of them looks
> >prefect to me.
> >
> >
> >As we have supported range-based for loop in our code, I purpose that we
> >have something like ReverseIntegerRange, and use this with range-based loop
> >to iterate the index. So we can use, for example: for (auto i :
> >ReverseIntegerRange(array.Length()))
> >
> >Maybe we can also add IntegerRange to benefit from the integer type
> >dedution.
> >
> >How does it sound?
>
> No, please don't do this. We need to make our container classes more
> similar to the STL containers, not less, and since this is about adding new
> functionality, here is what we need to do:

where the stl didn't make poor choices sure, and I'll agree while stl
iterators are a bit weird they aren't terrible.

> * Add a begin() and end() function to nsTArray that return iterators with
> STL semantics.
> * Add a rbegin() and rend() function to nsTArray with similar semantics
> which return a reverse iterator.
> * Add something similar to boost::adaptors::reverse, probably to MFBT.
>
> That way you can write:
>
> for (auto& elem : array)
>
> or:
>
> for (auto& elem : mozilla::Reverse(array))

imho auto in range based for should be strongly discouraged since you
need to go look up the type of the array and then play games to figure
out what the type of elem is.

Trev

>
> And that would work no matter whether array is an nsTArray or a std::vector.

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org

> https://lists.mozilla.org/listinfo/dev-platform