Extend base::Reversed to support iterating arrays

Extend base::Reversed to support iterating arrays

C++11's range-based for syntax supports iterating arrays,
so it makes sense to support arrays in base::Reversed too.

Committed: https://crrev.com/137978257f920ebdaa7d65493f39194a6a4a2e0c
Cr-Commit-Position: refs/heads/master@{#354181}

[mdempsky, 2015-09-24 06:41:14]

mdempsky@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2015-10-14 23:21:05]

Is this something that's provided by c++11 std library? I couldn't find it, but
the CL description is a bit vague like it might be?

https://codereview.chromium.org/1360423002/diff/1/base/containers/adapters.h
File base/containers/adapters.h (right):

https://codereview.chromium.org/1360423002/diff/1/base/containers/adapters.h#...
base/containers/adapters.h:43: // TODO(mdempsky): clang-format rewrites this as
"T(&t)[N]" (b/24351059).
Thanks for filing. Has the fix rolled to chrome yet?

[mdempsky, 2015-10-14 23:39:12]

On 2015/10/14 23:21:05, danakj wrote:
> Is this something that's provided by c++11 std library? I couldn't find it,
but
> the CL description is a bit vague like it might be?

Is it possible you're referring to the CL description that was in the review
email?  I originally wrote "C++11's range-based for syntax has special support
for iterating arrays in reverse order".  That final "in reverse order" bit was
erroneous, and I've removed it.

What I mean in the current CL description (but which might still be unclear) is
that currently this is valid:

    int array[10] = { ... };
    std::vector<int> vector = { ... };

    for (int x : vector) { ... }  // forward
    for (int x : array) { ... }  // forward
    for (int x : base::Reversed(vector)) { ... }  // reverse

This CL now also allows:

    for (int x : base::Reversed(array)) { ... }  // reverse

There's no C++11 standard library alternative to either form of base::Reversed
as far as I know.

https://codereview.chromium.org/1360423002/diff/1/base/containers/adapters.h
File base/containers/adapters.h (right):

https://codereview.chromium.org/1360423002/diff/1/base/containers/adapters.h#...
base/containers/adapters.h:43: // TODO(mdempsky): clang-format rewrites this as
"T(&t)[N]" (b/24351059).
On 2015/10/14 23:21:05, danakj wrote:
> Thanks for filing. Has the fix rolled to chrome yet?

Not yet, as far as I can tell.  Still, I removed the clang-format directives
optimistically hoping that it will be rolled before the next time someone has to
work in this file.

[danakj, 2015-10-14 23:40:28]

LGTM. I see, thanks! This is nice.

[mdempsky, 2015-10-14 23:44:51]

The CQ bit was checked by mdempsky@chromium.org

[mdempsky, 2015-10-14 23:45:00]

On 2015/10/14 23:40:28, danakj wrote:
> LGTM. I see, thanks! This is nice.

Thanks!

[commit-bot: I haz the power, 2015-10-14 23:45:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1360423002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1360423002/20001

[commit-bot: I haz the power, 2015-10-15 01:06:19]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-15 01:06:55]

Patchset 2 (id:??) landed as
https://crrev.com/137978257f920ebdaa7d65493f39194a6a4a2e0c
Cr-Commit-Position: refs/heads/master@{#354181}

[danakj, 2015-11-22 18:20:37]

On Wednesday, October 14, 2015, <mdempsky@chromium.org> wrote:

> On 2015/10/14 23:21:05, danakj wrote:
>
>> Is this something that's provided by c++11 std library? I couldn't find
>> it,
>>
> but
>
>> the CL description is a bit vague like it might be?
>>
>
> Is it possible you're referring to the CL description that was in the
> review
> email?  I originally wrote "C++11's range-based for syntax has special
> support
> for iterating arrays in reverse order".  That final "in reverse order" bit
> was
> erroneous, and I've removed it.
>
> What I mean in the current CL description (but which might still be
> unclear) is
> that currently this is valid:
>
>     int array[10] = { ... };
>     std::vector<int> vector = { ... };
>
>     for (int x : vector) { ... }  // forward
>     for (int x : array) { ... }  // forward
>     for (int x : base::Reversed(vector)) { ... }  // reverse
>
> This CL now also allows:
>
>     for (int x : base::Reversed(array)) { ... }  // reverse
>
> There's no C++11 standard library alternative to either form of
> base::Reversed
> as far as I know.


Apparently there is this in C++14 :)
http://en.cppreference.com/w/cpp/iterator/rbegin


>
>
>
> https://codereview.chromium.org/1360423002/diff/1/base/containers/adapters.h
> File base/containers/adapters.h (right):
>
>
>
https://codereview.chromium.org/1360423002/diff/1/base/containers/adapters.h#...
> base/containers/adapters.h:43: // TODO(mdempsky): clang-format rewrites
> this as "T(&t)[N]" (b/24351059).
> On 2015/10/14 23:21:05, danakj wrote:
>
>> Thanks for filing. Has the fix rolled to chrome yet?
>>
>
> Not yet, as far as I can tell.  Still, I removed the clang-format
> directives optimistically hoping that it will be rolled before the next
> time someone has to work in this file.
>
> https://codereview.chromium.org/1360423002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mdempsky, 2015-11-23 17:44:46]

On 2015/11/22 18:20:37, danakj (behind on reviews) wrote:
> Apparently there is this in C++14 :)
> http://en.cppreference.com/w/cpp/iterator/rbegin

Yep:

	 27   // TODO(mdempsky): Once we can use C++14 library features, use
std::rbegin
	 28   // and std::rend instead, so we can remove the specialization below.

One thing that occurs me now is we could backport std::r{begin,end} as
base::r{begin,end} and use them in base::Reversed.  Then once C++14 STL is
available, it's just a matter of deleting some code and
s/base(::r(begin|end))/std\1/g.

Worth preparing a CL for that, or just stick with what we have?