Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement proposal: verices() and edges() returning boost::iterator_range #122

Open
mkatliar opened this issue Oct 17, 2018 · 7 comments

Comments

@mkatliar
Copy link

Currently the vertices() and edges() functions in BGL return an std::pair of iterators. To iterate over vertices, a code like following is usually written:

typename graph_traits<Graph>::vertices_size_type b = 0;
typename graph_traits<Graph>::vertex_iterator i, end;
for (boost::tie(i, end) = vertices(g); i != end; ++i)
    do_something(*i);

If vertices() and edges() returned a boost::iterator_range, a cleaner code could be written:

for (auto v : vertices(g))
    do_something(v);
@anadon
Copy link
Contributor

anadon commented Oct 18, 2018

It would be a massive improvement, but would also require a fair bit of work since the old way is used all over graph. I'm working on a replacement that addresses that off of a from scratch code base which will do this. It has been on my radar.

@mkatliar
Copy link
Author

@anadon I could work on that change if it is approved and will be integrated in the library.

@anadon
Copy link
Contributor

anadon commented Oct 18, 2018

If you don't break the macros and existing code, I'll buy you a drink. It would be a welcomed improvement.

@mkatliar
Copy link
Author

mkatliar commented Oct 19, 2018

@anadon my concern is that it will break user's code relying on the old semantics. To keep it compatible, we probably need new names for these functions.

Or, those functions should return an object which has both old std::pair and the new boost::iterator_range semantics. Then the existing code relying on the old semantics can remain unchanged.

What do you think?

@anadon
Copy link
Contributor

anadon commented Oct 19, 2018

There are a few macros which would have to be preserved as well. One of the reasons why it is a big task.

@jeremy-murphy
Copy link
Contributor

std::pair of iterators satisfies the Boost Range concept: https://www.boost.org/doc/libs/master/libs/range/doc/html/range/introduction.html

So whilst it is true that you cannot use it in a C++11 range-for loop, you can use it with any Boost.Range algorithm.
E.g.:

#include <boost/range.hpp>
#include <boost/range/for_each.hpp>

// ...

boost::for_each(vertices(g), do_something);

Also, adapting std::pair to a standard readable range is not as difficult as you think, you can just wrap it in a call to boost::make_iterator_range like so:

for (auto const &vertex : boost::make_iterator_range(vertices(g)))
  do_something(vertex);

I agree that it would be nice if the result of vertices(g) were a standard readable range... but in my opinion it's a fairly low priority given what I have demonstrated above.

mkatliar added a commit to mkatliar/graph that referenced this issue Oct 23, 2018
@mkatliar
Copy link
Author

I discovered that there are some issues with using boost::iterator_range in combination with boost::counting_iterator: boostorg/range#82, boostorg/range#83. boost::counting_iterator is used as an implementation for some iterators in BGL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants