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

Add generic addNode and addEdge overloads #361

Closed
sbaldu opened this issue Oct 5, 2023 · 7 comments · Fixed by #363
Closed

Add generic addNode and addEdge overloads #361

sbaldu opened this issue Oct 5, 2023 · 7 comments · Fixed by #363
Assignees
Labels
Under Review Under Review

Comments

@sbaldu
Copy link
Collaborator

sbaldu commented Oct 5, 2023

I was thinking that it might be nice to add overloads for the addNode and addEdge methods that take an arbitrary number of nodes or edges.
This could be done with something like:

template <typename T1, typename... Tn>
void Graph<T>::addNode(T1 node, Tn... nodes) {

and in addition it would be nice to define a type trait that makes sure that the template parameter is in fact a node/edge, like:

template <typename T>
struct is_node : std::false_type {};

template <typename T>
struct is_node<const Node<T>*> : std::true_type {};

so that the complete function declaration becomes:

template <typename T1, typename... Tn>
requires is_node<T1>::value && (is_node<Tn>::value && ...)
void Graph<T>::addNode(T1 node, Tn... nodes) {

This might be an overkill but I think that it could be a useful addition;
What do you think @ZigRazor @nrkramer?

P.S. the use of requires needs C++20, but considering issue #266 I suppose that there was already the intention of updating the standard version.

@nrkramer
Copy link
Collaborator

nrkramer commented Oct 6, 2023

I don't think forcing users to upgrade their version of the C++ standard is necessarily a good thing.

Supporting new features from the standard is fine as long as it's hidden in the library - but if it's in the headers it becomes a far more troublesome problem.

Having the maximum amount of backwards compatibility is a good thing. Most library implementers should strive for that.

This being said, the template variadic list is a neat idea.

@nrkramer
Copy link
Collaborator

nrkramer commented Oct 6, 2023

I think we should discuss and consider the possibility of distributing a standard hpp/cpp library, as the drawbacks of header-only are starting to manifest in this library.

@nrkramer
Copy link
Collaborator

nrkramer commented Oct 6, 2023

This SO article goes over some of the basic points: https://softwareengineering.stackexchange.com/questions/305618/are-header-only-libraries-more-efficient

I do think the low administrative costs of a header-only library make it attractive to keep.

@ZigRazor
Copy link
Owner

ZigRazor commented Oct 6, 2023

I think we should discuss and consider the possibility of distributing a standard hpp/cpp library, as the drawbacks of header-only are starting to manifest in this library.

I think we can open a discussion in discussion section.

@ZigRazor
Copy link
Owner

ZigRazor commented Oct 6, 2023

I do think the low administrative costs of a header-only library make it attractive to keep.

I Agree with @nrkramer

@ZigRazor ZigRazor added the Under Review Under Review label Oct 6, 2023
@sbaldu
Copy link
Collaborator Author

sbaldu commented Oct 6, 2023

This being said, the template variadic list is a neat idea.

If you think that it would be a useful addition we could still implement the new method without using the C++20 requires.
I think that we can implement the check on the template parameters by simply using std::enable_if, which is from C++11 so we don't need to update the standard.
What do you think?

@ZigRazor
Copy link
Owner

ZigRazor commented Oct 6, 2023

In this way, I think is good!

@sbaldu sbaldu self-assigned this Oct 6, 2023
@sbaldu sbaldu linked a pull request Oct 8, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Under Review Under Review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants