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

Graph concepts do not work with reference types #8

Open
BenBrock opened this issue Jan 19, 2023 · 0 comments
Open

Graph concepts do not work with reference types #8

BenBrock opened this issue Jan 19, 2023 · 0 comments

Comments

@BenBrock
Copy link

BenBrock commented Jan 19, 2023

The NWGraph graph concepts do not work with reference types. For example, the following code fails to compile:

#include <nwgraph/edge_list.hpp>

// Fails: G = nw::graph::edge_list<...>& does not fulfill `nw::graph::edge_list_graph`
template <nw::graph::edge_list_graph G>
void foo(G&&) {}

int main(int argc, char** argv) {
  nw::graph::edge_list<nw::graph::directedness::undirected, float> graph(0);

  foo(graph);

  return 0;
}

Since graphs are specializations of ranges and will often represent data structures you don't want to copy, I think it will often be useful to use forwarding references (as above) with them. Note that std::ranges concepts like range, forward_range, and random_access_range do work with references.

(For counterpoint, the iterator concepts do not work with references, which has always annoyed me. However, iterators are supposed to be cheap to copy, so I suppose the standard library is implicitly encouraging us to pass iterators by value. However, I suppose you generally won't want to pass graphs by value.)

The two issues preventing references from working are:

  • vertex_id_t This could be fixed by changing the definition on line 37 of graph_traits.hpp to
template <typename G>
using vertex_id_t = typename graph_traits<std::remove_cvref_t<G>>::vertex_id_type;
  • std::copyable You could also add a std::remove_cvref_t<G> on line 57 of graph_concepts.hpp. However, I'm not sure that I'm convinced graphs always need be copyable in the first place? I suppose both a container, which owns the data, as well as a copy-constructible view, would be copyable, although copying means different things in these two cases. I guess you're really disabling move-only views from being graphs here?
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

1 participant