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

labeled_graph::remove_vertex does not remove label #167

Open
arneboe opened this issue Apr 16, 2019 · 20 comments
Open

labeled_graph::remove_vertex does not remove label #167

arneboe opened this issue Apr 16, 2019 · 20 comments
Assignees
Labels

Comments

@arneboe
Copy link
Contributor

arneboe commented Apr 16, 2019

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;
}
@anadon
Copy link
Contributor

anadon commented Apr 17, 2019

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

@jzmaddock
Copy link
Contributor

Go for it!

@anadon
Copy link
Contributor

anadon commented Apr 17, 2019

@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.

@arneboe
Copy link
Contributor Author

arneboe commented Apr 18, 2019

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

@anadon
Copy link
Contributor

anadon commented Apr 18, 2019

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
Copy link
Contributor

anadon commented Apr 18, 2019

@anadon
Copy link
Contributor

anadon commented Apr 18, 2019

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

@anadon
Copy link
Contributor

anadon commented Apr 24, 2019

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

@HWiese1980
Copy link

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.

@anadon
Copy link
Contributor

anadon commented Jul 11, 2019

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

@HWiese1980
Copy link

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

@fjch1997
Copy link

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

@HWiese1980
Copy link

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

@jeremy-murphy
Copy link
Contributor

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.

@EduardoGoulart1
Copy link

EduardoGoulart1 commented Jun 29, 2023

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

@DeuceBox
Copy link

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:
DeuceBox@de651cb

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

@jeremy-murphy jeremy-murphy self-assigned this Jul 7, 2023
@StahlTim
Copy link

@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.

@DeuceBox
Copy link

@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.

@jeremy-murphy
Copy link
Contributor

I'll prioritise merging in the fix asap.

@jeremy-murphy
Copy link
Contributor

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

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

No branches or pull requests

9 participants