graph icon indicating copy to clipboard operation
graph copied to clipboard

labeled_graph::remove_vertex does not remove label

Open arneboe opened this issue 5 years ago • 23 comments

When calling remove_vertex on a labeled_graph the vertex is removed but the label is not. A dangling reference to the vertex is left behind. Accessing it causes segfaults.

Minimal example:

#include <iostream>
#include <string>

#include <boost/graph/directed_graph.hpp>
#include <boost/graph/labeled_graph.hpp>

using namespace boost;
using namespace std;

int main() {

    using namespace boost::graph_detail;
    typedef directed_graph<> Digraph;
    typedef labeled_graph<Digraph, string> Graph;
    
    Graph g;
    g.add_vertex("foo");
    
    auto v = g.vertex("foo");
    if(v != nullptr)
        cout << "foo exists\n";
    
    g.remove_vertex("foo");
    
    auto v2 = g.vertex("foo");
    if(v2 != nullptr)
        std::cout << " BUG! vertex label still exists.\n";


    return 0;
}

arneboe avatar Apr 16 '19 09:04 arneboe

@jzmaddock Hey, this looks like a good one for me to work on.

anadon avatar Apr 17 '19 04:04 anadon

Go for it!

jzmaddock avatar Apr 17 '19 16:04 jzmaddock

@arneboe Can I get your full name and email, since what you submitted is what I'm basing the regression test on? I need to credit you.

anadon avatar Apr 17 '19 19:04 anadon

I dont care about the credit, but if you have to: Arne Böckmann

arneboe avatar Apr 18 '19 07:04 arneboe

It looks like the intended design, given the state of BGL, is to have _map a protected member, not a private member. So you hack was less of a hack than was expected.

anadon avatar Apr 18 '19 18:04 anadon

This may take longer than expected: https://github.com/boostorg/graph/blob/develop/include/boost/graph/adjacency_matrix.hpp#L992

anadon avatar Apr 18 '19 19:04 anadon

May be related: https://svn.boost.org/trac10/ticket/7863

anadon avatar Apr 18 '19 19:04 anadon

This is going to be delayed somewhat. There are general cleanups in testing which I need to do first.

anadon avatar Apr 24 '19 17:04 anadon

Sure, take your time! Better have it fixed correctly and thoroughly, than to rush it and only come up with some quick'n'dirty patch that fails somewhere else.

HWiese1980 avatar Apr 24 '19 18:04 HWiese1980

Just an update, had to move twice, once across states, and started a new job. Getting back to this more now.

anadon avatar Jul 11 '19 00:07 anadon

Alright, no pressure. Just don't forget it ;-)

HWiese1980 avatar Jul 24 '19 05:07 HWiese1980

And... it was forgotten This bug really tripped me up when I tried to use labeled_graph

fjch1997 avatar Jul 29 '22 18:07 fjch1997

As it seems... @anadon how's it going?

HWiese1980 avatar Jul 30 '22 05:07 HWiese1980

Not sure if they are still active on Boost.Graph, but if someone submits a pull request to fix this issue, I can help them get it polished and merge it.

jeremy-murphy avatar Aug 01 '22 00:08 jeremy-murphy

Hey there, any update on it? @anadon by any chance do you still have your WIP PR that people can pick up from?

EduardoGoulart1 avatar Jun 29 '23 09:06 EduardoGoulart1

This may be related:

There was a patch at https://svn.boost.org/trac/boost/ticket/9493 (I did not create it) which is no longer reachable, but I've been using it in production for years

I've preserved it here: https://github.com/DeuceBox/graph/commit/de651cb06cd92d59effed88ae0fc79852c43ff34

It appears to fix the bug in the minimal example given in the OP.

DeuceBox avatar Jun 30 '23 14:06 DeuceBox

@DeuceBox @jeremy-murphy Is there currently an active pull request introducing the fix referenced by @DeuceBox?

Facing the same issue and would appreciate the fix being shipped with the stock boost library.

StahlTim avatar Nov 17 '23 15:11 StahlTim

@StahlTim @jeremy-murphy since there wasn't any real discussion beyond the branch/fix I linked a while back, I've put in a PR.

DeuceBox avatar Nov 17 '23 17:11 DeuceBox

I'll prioritise merging in the fix asap.

jeremy-murphy avatar Nov 18 '23 09:11 jeremy-murphy

Could someone please check out the fix I just merged and test it? Thanks to @DeuceBox for making the PR.

jeremy-murphy avatar Nov 28 '23 00:11 jeremy-murphy

Hi @jeremy-murphy, has this fix already been merged somewhere? I can't find it in the latest release

EduardoGoulart1 avatar Jul 26 '24 13:07 EduardoGoulart1

@EduardoGoulart1 It's merged here https://github.com/boostorg/graph/pull/354 in the "develop" branch. The boostorg/graph "develop" branch is what the boostorg/boost "master" branch references. However, the latest boostorg/boost release branch/tag (boost-1.85.0) references the corresponding release tag in boostorg/graph (https://github.com/boostorg/graph/releases/tag/boost-1.85.0), but it appears based on https://github.com/boostorg/graph/tags that the boostorg/graph repo's release tags haven't pointed to anything new since 2022.

@jeremy-murphy could we get the next boostorg/graph release tag to reference a more recent version of develop? I don't know what's involved in that process.

DeuceBox avatar Jul 26 '24 15:07 DeuceBox

Thanks for pointing it out. That's about how long I've been maintainer for and I'm still learning what needs to be done! I assumed all that release tag business would be automatic from the super project.

jeremy-murphy avatar Jul 27 '24 05:07 jeremy-murphy