Revert change to reindex_edge_list.

[cqc-alec]

Fixes #268 .

[prince-chrismc]

You should be able to keep the for loop and remove the increments if you correctly update after the erease

[prince-chrismc]

Suggested change

	el.erase(ce);
	ei = el.erase(ce);

[cqc-alec]

Thanks for the suggestion. It indeed looks like a better approach; however, I was not able to get this code to compile:

#include <boost/graph/adjacency_list.hpp>

using Graph = boost::adjacency_list<boost::setS, boost::vecS, boost::undirectedS>;

int main() {
  Graph g(3);
  boost::add_edge(2, 0, g);
  boost::remove_vertex(1, g);
}

The gist of the error is that erase does not return an iterator in this case, but a long unsigned int.

(The original patch was simply a reversion of a previous commit.)

[jeremy-murphy]

Could you please add the example from the bug report that can trigger this segfault as a new unit test?
As long as it is reliable under valgrind or a memory sanitizer, that is sufficient.

[cqc-alec]

Could you please add the example from the bug report that can trigger this segfault as a new unit test? As long as it is reliable under valgrind or a memory sanitizer, that is sufficient.

Yes, I will do.

[cqc-alec]

I have added the unit test to delete_edge.cpp, which seems the most appropriate existing test file -- is that OK or would you prefer a new source file?

[jeremy-murphy]

I have added the unit test to delete_edge.cpp, which seems the most appropriate existing test file -- is that OK or would you prefer a new source file?

I think the location is fine.

Is there a reason that you can't use the existing m_graph object? I know the MWE given uses undirected edges but the explanation does not stipulate it. Hmm. Anyway, it's probably good to test on both directed and undirected.

I know that we're primarily testing for a segfault, but it looks weird in a test if there is no accompanying assertion, so please add in a BOOST_TEST for i) the expected number of vertices and ii) edges after calling remove_vertex.

Thank you!!

[jeremy-murphy]

Looks great, thanks. As soon as the CI is fixed, I'll merge this.

[jeremy-murphy]

Please update to the latest changes on develop so we can test for a green build.

[cqc-alec]

Please update to the latest changes on develop so we can test for a green build.

Done. Still seems to be one build failure (looks unrelated) ...?

[jeremy-murphy]

Yeah, that is strange and annoying.

[jeremy-murphy]

Might be a real issue with Boost.Filesystem, let's see: boostorg/filesystem#227

[jeremy-murphy]

The simplest way I know to re-run the CI is to close the PR and open it again!

[jeremy-murphy]

Hmmm, that didn't work as I expected.

[jeremy-murphy]

A couple of issues have been fixed so I think if you merge develop now, you should get a green build.

[cqc-alec]

A couple of issues have been fixed so I think if you merge develop now, you should get a green build.

Done: seems good now!

[jeremy-murphy]

Awesome!

In the future, I think it's good to refer to the commit that you're reverting (I put it in the merge commit), so that people can easily track the history.