Range-based for loops in reverse?

[Matthew Dempsky]

It seems like a non-trivial amount of Chromium code wants to iterate containers in reverse: https://code.google.com/p/chromium/codesearch#search/&q=for.*rbegin&sq=package:chromium&type=cs

Do we have anything in base to make that easier? Is that something worth adding?

E.g., this seems to work in some quick testing:

template <typename T>

T& declval();

template <typename T>

class ReverseImpl {

 public:

  explicit ReverseImpl(T& t) : t_(t) {}

  friend decltype(declval<T>().rbegin()) begin(ReverseImpl& ri) { return ri.t_.rbegin(); }

  friend decltype(declval<T>().rend()) end(ReverseImpl& ri) { return ri.t_.rend(); }

 private:

  T& t_;

};

template <typename T>

ReverseImpl<T> reverse(T& t) {

  return ReverseImpl<T>(t);

}

[James Robinson]

On Fri, Sep 26, 2014 at 1:56 PM, Matthew Dempsky <mdem...@chromium.org> wrote:

It seems like a non-trivial amount of Chromium code wants to iterate containers in reverse: https://code.google.com/p/chromium/codesearch#search/&q=for.*rbegin&sq=package:chromium&type=cs

Do we have anything in base to make that easier? Is that something worth adding?

What's the syntax you want to use?

  std::vector<int> v;

  v.push_back(1);

  v.push_back(2);

  v.push_back(3);

  for (auto it = v.rbegin(); it != v.rend(); ++it) {

    printf("%d\n", *it);

  }

seems pretty easy to me...

- James

[Yuri Wiitala]

+1: Would like to see a reverse_iterator_adapter in base/stl_util.h.

BTW--There are a few good proposals for solving this problem here: http://stackoverflow.com/questions/8542591/c11-reverse-range-based-for-loop

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Yuri Wiitala]

To be clear, it'd be nice if code code be written as something like:

for (const auto& e : base::STLReverse(container)) {

  // do something

[James Robinson]

On Fri, Sep 26, 2014 at 2:10 PM, Yuri Wiitala <m...@chromium.org> wrote:

To be clear, it'd be nice if code code be written as something like:

for (const auto& e : base::STLReverse(container)) {

  // do something

}

That doesn't seem like enough of an improvement for the complexity required by such an adapter.

- James

[Peter Kasting]

The complexity required at the definition spot?  That seems like a small fixed cost.  Matthew's proposed implementation, for example, seems pretty small and reasonable.

It seems like the improvement is equal to the improvement we get from range-based for in the general forward-iterating case.  I'd like to see such an adapter.

PK

[Yuri Wiitala]

Sorry, I should have clarified.  I think the code represented by "// do something" in my example would have a lot to gain in terms of simplicity, and not necessarily the "for (...)" part, as you pointed out.

[Victor Khimenko]

Are we sure we want to have such adapter and not just boost::adaptors set ?

  http://www.boost.org/doc/libs/1_56_0/libs/range/doc/html/range/reference/adaptors/reference.html

There are boost::adaptors::reverse and other such stuff.

[Matthew Dempsky]

On Fri, Sep 26, 2014 at 2:04 PM, James Robinson <jam...@chromium.org> wrote:

What's the syntax you want to use?

for (int x : reverse(v)) {

  printf("%d\n", x);

}

[Alex Vakulenko]

+1 for the adapter. Specific implementation TBD though :)

--

[Chris Hopman]

I would like this for two reasons:

1. consistency. It would be jarring for me to come across a non-range based for loop in code that uses range-based for everywhere else. Here's some examples where we use both forward and reverse iteration near each other:

ash/display/display_manager.cc:929-946

ash/wm/workspace/workspace_window_resizer.cc:756-770

athena/wm/window_overview_mode:window_overview_mode.cc:363-380 (this is particular interesting because it is actually iterating over the same thing in different directions)

chrome/browser/apps/app_shim/extension_app_shim_handler_mac.cc:56-71

2. it makes it much more obvious that you are actually iterating in reverse. Take a look at the workspace_window_resizer.cc example. Currently the only thing that really calls out that that iteration is done in reverse is the const_reverse_iterator type name (I don't think the 'r' prefix on begin/end is obvious). Now imagine that one using 'auto'.

On Fri, Sep 26, 2014 at 2:17 PM, 'Matthew Dempsky' via Chromium-dev <chromi...@chromium.org> wrote:

--

[Hendrik]

+1 for adapter, this is the correct way to handle this situation.

I've worked on code that provided one in the past, it is quite handle and it documents the code at the same time.

I would suggest base::backwards(v), reverse already means something different in c++.

On Friday, September 26, 2014 2:20:29 PM UTC-7, Alex wrote:

+1 for the adapter. Specific implementation TBD though :)

On Fri, Sep 26, 2014 at 2:17 PM, 'Matthew Dempsky' via Chromium-dev wrote:

[Matt Perry]

On Fri, Sep 26, 2014 at 2:25 PM, Hendrik <hend...@chromium.org> wrote:

I would suggest base::backwards(v), reverse already means something different in c++.

Or base::reversed(v), like python: https://docs.python.org/2/library/functions.html#reversed

[Matthew Dempsky]

On Fri, Sep 26, 2014 at 2:28 PM, Matt Perry <mpcom...@chromium.org> wrote:

Or base::reversed(v), like python: https://docs.python.org/2/library/functions.html#reversed

I like base::reversed.

https://codereview.chromium.org/605243003/

[Brett Wilson]

If we want this (I'm not taking a position), I don't think we should
add it to stl_util which is currently a weird subset of the internal
Google version of this file.

Maybe base/containers/adapters.h

[Alex Vakulenko]

Whatever you do, don't call it base:STLSomething() :)

--

[Alex Vakulenko]

+1 for base/containers/adapters.h

[Yuri Wiitala]

On Fri Sep 26 2014 at 2:55:22 PM Alex Vakulenko <avaku...@chromium.org> wrote:

Whatever you do, don't call it base:STLSomething() :)

You don't like: base::containers::util::STLValuePointersInReverseOooohYeah()

;-)

 

[Victor Khimenko]

Well hopefully soon we'll be able to do "template<typename T> using reversed = base::containers::util::STLValuePointersInReverseOooohYeah<T>" thus the actual name of that type will not matter as much.

P.S. But yes, I would still like something less verbose anyway.

[Alex Vakulenko]

I think it should be "base::containers::stl_utils::stl_container_adaptors::STLValuePointersInReverseOooohYeah<T>" just to make sure...

[Matthew Dempsky]

On Fri, Sep 26, 2014 at 2:55 PM, Alex Vakulenko <avaku...@chromium.org> wrote:

Whatever you do, don't call it base:STLSomething() :)

FYI, it's committed now as base::Reversed(), provided by "base/containers/adapters.h".

Cheers