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

std::max_element requires forward iterator #175

Open
rogeeff opened this issue Jul 12, 2019 · 4 comments
Open

std::max_element requires forward iterator #175

rogeeff opened this issue Jul 12, 2019 · 4 comments

Comments

@rogeeff
Copy link

rogeeff commented Jul 12, 2019

std::max_element contract required forward iterator. Graph library's uses this interface in betweenness_centrality_clustering on edge iterators, which according to boost iterator library are deduced as input iterators. This fails to build with libc++, which validates the category at compile time.

The easiest solution is to handcraft the for loop:

auto it = edges_iters.first;
auto it_max = it;
while (++it != edges_iters.second) {
  if (cmp(*it_max, *it)) it_max = it;
}
// Here we assume graph is not empty and contains at least one edge.
edge_descriptor e = *it_max;
@anadon
Copy link
Contributor

anadon commented Jul 13, 2019

Hello,

I'm getting back to this repo after a month and a half. While it is possible to make a loop in user code, that might not be the right option. Though it may also be correct since a forward iterator may not make sense in a graph since it isn't convertible in a reliable way to a 1D directional structure. I am working on Travis CI stuff right now and have a backlog, but I'll address this when I can. If you would like a faster resolution and feel comfortable doing so, I will welcome you to create a test case and a code fix in a PR.

@morinmorin
Copy link
Member

C++20's new overloads of std::max_element would work. But, no need to wait for C++20! We already have boost::range::max_element (or boost::first_max_element in Boost.Algorithm) that works with ForwardTraversal ranges (iterators).

@bkpoon
Copy link
Contributor

bkpoon commented Oct 7, 2019

I ran into this issue while trying to compile our code using Boost.Graph on macOS with Xcode 11. It looks like Boost.Range did what @morinmorin suggested by replacing std::max_element with boost::first_max_element (boostorg/range@adcb071).

For testing our code, I modified include/boost/graph/bc_clustering.hpp to add the <boost/algorithm/minmax_element.hpp> header, removed the <algorithm> header, and replaced line 136

        = *max_element(edges_iters.first, edges_iters.second, cmp);

with

        = *boost::first_max_element(edges_iters.first, edges_iters.second, cmp);

I'm still testing this on the other platforms and compilers that we support, though. I can submit a pull request if that helps.

benmwebb added a commit to salilab/imp that referenced this issue Oct 9, 2019
clang fails to compile boost::betweenness_centrality_clustering
complaining that the edge iterators are not forward iterators,
as described at boostorg/graph#175. Work around by using
boost::first_max_element instead of std::max_element. This should
fix compilation on Macs with Homebrew.
@bkpoon
Copy link
Contributor

bkpoon commented Oct 16, 2019

I made a pull request (#190). There was another file, include/boost/graph/howard_cycle_ratio.hpp, that has the same issue, so I patched that as well.

bkpoon added a commit to bkpoon/graph that referenced this issue Oct 16, 2019
bkpoon added a commit to cctbx/cctbx_project that referenced this issue Nov 25, 2019
- Patch bc_clustering.hpp header
- boostorg/graph#175
- Can be removed after Boost.Graph is patched
bkpoon added a commit to cctbx/cctbx_project that referenced this issue Nov 25, 2019
- Patch bc_clustering.hpp header
- boostorg/graph#175
- Can be removed after Boost.Graph is patched
bkpoon added a commit to cctbx/cctbx_project that referenced this issue Nov 27, 2019
- Patch bc_clustering.hpp header
- boostorg/graph#175
- Can be removed after Boost.Graph is patched
bkpoon added a commit to cctbx/cctbx_project that referenced this issue Dec 2, 2019
- Patch bc_clustering.hpp header
- boostorg/graph#175
- Can be removed after Boost.Graph is patched
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

4 participants