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

Apply std::remove_const before passing Graph to graph_traits in filtered_graph #395

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

tsnorri
Copy link
Contributor

@tsnorri tsnorri commented Nov 8, 2024

For instance detail::residual_graph explicitly applies const to the graph. Consequently graph_traits would have to be defined also for the const graph type, but the LEDA graph example does not seem to indicate that it should be needed.

– E.g. detail::residual_graph explicitly applies const to the graph. Consequently graph_traits would have to be defined also for the const graph type.
@tsnorri
Copy link
Contributor Author

tsnorri commented Nov 8, 2024

I just noticed that the language version in use seems to be C++14. I can change the instances of std::remove_const to std::remove_const_t if needed.

@tsnorri tsnorri changed the title Apply std::remove_const before passing Graph to graph_traits Apply std::remove_const before passing Graph to graph_traits in filtered_graph Nov 8, 2024
@jeremy-murphy
Copy link
Contributor

Hmmm, I would only do this if I'm 100% sure that the underlying graph can never actually be const, and it feels hacky and wrong. Funnily enough, there is a comment at the top of filtered_graph_base:

// This base class is a stupid hack to change overload resolution
// rules for the source and target functions so that they are a
// worse match than the source and target functions defined for
// pairs in graph_traits.hpp. I feel dirty. -JGS

I would much rather someone redesigned filtered_graph to work elegantly, which might be much easier now with C++14 metaprogramming.

@tsnorri
Copy link
Contributor Author

tsnorri commented Nov 11, 2024

The underlying graph is in fact const at least when using cycle_canceling, since it eventually calls residual_graph which applies const to the graph type. I think the reason why the function works with adjacency_list is that the default implementation of graph_traits (which is here I think) gets the types from the graph type, so it does not matter if it is const or not.

(This has more to do with my other PR but anyway if the graph does not have a particular type, e.g. in_edge_iterator, the default implementation of graph_traits just adds void. For example this compiles:

namespace {
    struct test
    {
        typedef int vertex_descriptor;
        typedef int edge_descriptor;
        typedef boost::directed_tag directed_category;
        typedef boost::allow_parallel_edge_tag edge_parallel_category;
        typedef boost::incidence_graph_tag traversal_category;
        typedef std::size_t vertices_size_type;
        typedef std::size_t edges_size_type;
    };

    typedef boost::graph_traits <test> traits;
    static_assert(std::is_same_v <traits::in_edge_iterator, void>);
}

)

I don’t think I’m familiar enough with BGL to come up with a better design for filtered_graph, so would you have any advice for now to get my code to work? Should I specialise graph_traits for the const version of my custom graph type, too?

@jeremy-murphy
Copy link
Contributor

Yeah, I'm not sure, to be honest. I would have to spend a while looking at it first and I don't have time to.

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

Successfully merging this pull request may close these issues.

2 participants